From e3c3261ce910fa419a18d574752dac46a19bade7 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Mon, 8 May 2017 15:23:55 +0200 Subject: [PATCH 01/30] Add find methods to user manager and group manager --- .../lib/Controller/ShareesController.php | 18 +++---- core/Migrations/Version20170510143952.php | 42 +++++++++++++++ lib/private/Group/Manager.php | 52 +++++++++++++++++++ lib/private/User/Account.php | 1 + lib/private/User/AccountMapper.php | 20 +++++++ lib/private/User/Backend.php | 2 + lib/private/User/Manager.php | 19 +++++++ lib/private/User/SyncService.php | 4 ++ lib/public/IGroupManager.php | 12 +++++ lib/public/IUserManager.php | 11 ++++ 10 files changed, 172 insertions(+), 9 deletions(-) create mode 100644 core/Migrations/Version20170510143952.php diff --git a/apps/files_sharing/lib/Controller/ShareesController.php b/apps/files_sharing/lib/Controller/ShareesController.php index 1a58ce204ad9..15d5da923cba 100644 --- a/apps/files_sharing/lib/Controller/ShareesController.php +++ b/apps/files_sharing/lib/Controller/ShareesController.php @@ -144,17 +144,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; } } @@ -164,13 +164,13 @@ 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, @@ -178,7 +178,7 @@ protected function getUsers($search) { ]; } else { $this->result['users'][] = [ - 'label' => $userDisplayName, + 'label' => $user->getDisplayName(), 'value' => [ 'shareType' => Share::SHARE_TYPE_USER, 'shareWith' => $uid, diff --git a/core/Migrations/Version20170510143952.php b/core/Migrations/Version20170510143952.php new file mode 100644 index 000000000000..8cf20baba204 --- /dev/null +++ b/core/Migrations/Version20170510143952.php @@ -0,0 +1,42 @@ + + * + * @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 + * + */ +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 additional_search_string column + $table->addColumn('searchString', 'string', [ + 'notnull' => false, + 'length' => 64, + ]); + } +} diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 55ecbe845906..1eed21471885 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -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; + } + die(var_dump($matchingUsers)); + return $matchingUsers; + } + /** * get a list of all display names in a group * @param string $gid diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index 4b927573b058..c7326bb2250f 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -64,6 +64,7 @@ class Account extends Entity { protected $backend; protected $state; protected $home; + protected $searchString; public function __construct() { $this->addType('state', 'integer'); diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 27a5a2acb76e..1761f33dfcb8 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -79,6 +79,26 @@ 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()) + // TODO we can add a config option to allow search by these fields + ->where($qb->expr()->iLike('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('additional_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`')]) diff --git a/lib/private/User/Backend.php b/lib/private/User/Backend.php index cbd984f94436..0d3e652728b3 100644 --- a/lib/private/User/Backend.php +++ b/lib/private/User/Backend.php @@ -46,6 +46,7 @@ abstract class Backend implements UserInterface { const SET_DISPLAYNAME = 1048576; // 1 << 20 const PROVIDE_AVATAR = 16777216; // 1 << 24 const COUNT_USERS = 268435456; // 1 << 28 + const SEARCH_STRING = 4294967296; // 1 << 32 protected $possibleActions = [ self::CREATE_USER => 'createUser', @@ -56,6 +57,7 @@ abstract class Backend implements UserInterface { self::SET_DISPLAYNAME => 'setDisplayName', self::PROVIDE_AVATAR => 'canChangeAvatar', self::COUNT_USERS => 'countUsers', + self::SEARCH_STRING => 'searchString', ]; /** diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 59dfc26a6e8e..f84b3db43788 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -241,6 +241,25 @@ 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 * diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 6fb5e5171511..43d2e74b166b 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -161,6 +161,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 supplys additional search strings + if ($this->backend->implementsActions(\OC_User_Backend::SEARCH_STRING)) { + $a->setSearchString($this->backend->getSearchString($uid)); + } return $a; } diff --git a/lib/public/IGroupManager.php b/lib/public/IGroupManager.php index 24a599f05cd0..61f444773969 100644 --- a/lib/public/IGroupManager.php +++ b/lib/public/IGroupManager.php @@ -123,6 +123,18 @@ public function getUserGroupIds($user, $scope = null); */ public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0); + /** + * search for users in a specific group + * + * @param string $gid + * @param string $search + * @param int $limit + * @param int $offset + * @return array an array of display names (value) and user ids (key) + * @since 10.0.1 + */ + public function findUsersInGroup($gid, $search = '', $limit = -1, $offset = 0); + /** * Checks if a userId is in the admin group * @param string $userId diff --git a/lib/public/IUserManager.php b/lib/public/IUserManager.php index cab2258c4d62..91186e051920 100644 --- a/lib/public/IUserManager.php +++ b/lib/public/IUserManager.php @@ -110,6 +110,17 @@ public function checkPassword($loginName, $password); */ public function search($pattern, $limit = null, $offset = null); + /** + * find a user account by checking user_id, display name and email fields + * + * @param string $pattern + * @param int $limit + * @param int $offset + * @return \OCP\IUser[] + * @since 10.0.1 + */ + public function find($pattern, $limit = null, $offset = null); + /** * search by displayName * From 53bef4a18a7e4607107aed2815b69ba0fe776e11 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Thu, 11 May 2017 10:58:22 +0100 Subject: [PATCH 02/30] Add indexes for improved search performance and use lower uid in query --- core/Migrations/Version20170510143952.php | 17 ++++++- lib/private/User/Account.php | 3 +- lib/private/User/AccountMapper.php | 5 +-- lib/private/User/Backend.php | 2 - lib/private/User/Manager.php | 8 ++++ lib/private/User/SyncService.php | 10 ++--- lib/private/User/User.php | 2 +- lib/public/IUser.php | 1 + ...videsAdditionalSearchAttributesBackend.php | 44 +++++++++++++++++++ 9 files changed, 77 insertions(+), 15 deletions(-) create mode 100644 lib/public/User/IProvidesAdditionalSearchAttributesBackend.php diff --git a/core/Migrations/Version20170510143952.php b/core/Migrations/Version20170510143952.php index 8cf20baba204..6fcd13883817 100644 --- a/core/Migrations/Version20170510143952.php +++ b/core/Migrations/Version20170510143952.php @@ -33,10 +33,23 @@ public function changeSchema(Schema $schema, array $options) { $prefix = $options['tablePrefix']; $table = $schema->getTable("{$prefix}accounts"); - // Add additional_search_string column - $table->addColumn('searchString', 'string', [ + // Add column to hold additional search attributes + $table->addColumn('search_attributes', 'string', [ 'notnull' => false, 'length' => 64, ]); + + // Add index for search attributes + $table->addIndex(['search_attributes'], 'search_attributes_index'); + + // 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'); + } } diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index c7326bb2250f..92fde74c619d 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -45,6 +45,7 @@ * @method void setQuota(string $quota) * @method string getHome() * @method void setHome(string $home) + * @method void setSearchAttributes(string $searchAttributes) * * @package OC\User */ @@ -64,7 +65,7 @@ class Account extends Entity { protected $backend; protected $state; protected $home; - protected $searchString; + protected $searchAttributes; public function __construct() { $this->addType('state', 'integer'); diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 1761f33dfcb8..897c1040f9f6 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -89,11 +89,10 @@ public function find($pattern, $limit, $offset) { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) - // TODO we can add a config option to allow search by these fields - ->where($qb->expr()->iLike('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->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) . '%'))) ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->iLike('additional_search_attributes', $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); diff --git a/lib/private/User/Backend.php b/lib/private/User/Backend.php index 0d3e652728b3..cbd984f94436 100644 --- a/lib/private/User/Backend.php +++ b/lib/private/User/Backend.php @@ -46,7 +46,6 @@ abstract class Backend implements UserInterface { const SET_DISPLAYNAME = 1048576; // 1 << 20 const PROVIDE_AVATAR = 16777216; // 1 << 24 const COUNT_USERS = 268435456; // 1 << 28 - const SEARCH_STRING = 4294967296; // 1 << 32 protected $possibleActions = [ self::CREATE_USER => 'createUser', @@ -57,7 +56,6 @@ abstract class Backend implements UserInterface { self::SET_DISPLAYNAME => 'setDisplayName', self::PROVIDE_AVATAR => 'canChangeAvatar', self::COUNT_USERS => 'countUsers', - self::SEARCH_STRING => 'searchString', ]; /** diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index f84b3db43788..175ccd849ab0 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -38,7 +38,9 @@ use OCP\IUser; use OCP\IUserManager; use OCP\IConfig; +use OCP\User\IProvidesAdditionalSearchAttributesBackend; use OCP\User\IProvidesEMailBackend; +use OCP\User\IProvidesQuotaBackend; use OCP\UserInterface; use Symfony\Component\EventDispatcher\GenericEvent; @@ -433,6 +435,12 @@ private function newAccount($uid, $backend) { $account->setQuota($quota); } } + if ($backend instanceof IProvidesAdditionalSearchAttributesBackend) { + $searchString = $backend->getSearchAttributes($uid); + if ($searchString !== null) { + $account->setSearchAttributes($searchString); + } + } $home = false; if ($backend->implementsActions(Backend::GET_HOME)) { $home = $backend->getHome($uid); diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 43d2e74b166b..fc1c5fcbd37f 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -18,14 +18,12 @@ * along with this program. If not, see * */ - - namespace OC\User; - use OCP\AppFramework\Db\DoesNotExistException; use OCP\IConfig; use OCP\ILogger; +use OCP\User\IProvidesAdditionalSearchAttributesBackend; use OCP\UserInterface; /** @@ -161,9 +159,9 @@ public function setupAccount(Account $a, $uid) { if ($this->backend->implementsActions(\OC_User_Backend::GET_DISPLAYNAME)) { $a->setDisplayName($this->backend->getDisplayName($uid)); } - // Check if backend supplys additional search strings - if ($this->backend->implementsActions(\OC_User_Backend::SEARCH_STRING)) { - $a->setSearchString($this->backend->getSearchString($uid)); + // Check if backend supplies an additional search string + if ($this->backend instanceof IProvidesAdditionalSearchAttributesBackend) { + $a->setSearchAttributes($this->backend->getSearchAttributes($uid)); } return $a; } diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 60137157bf16..55bc09a57ffd 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -60,7 +60,7 @@ class User implements IUser { /** @var IURLGenerator */ private $urlGenerator; - /** @var EventDispatcher */ + /** @var EventDispatcher */ private $eventDispatcher; /** @var AccountMapper */ diff --git a/lib/public/IUser.php b/lib/public/IUser.php index d8f6896c198d..5df691d73384 100644 --- a/lib/public/IUser.php +++ b/lib/public/IUser.php @@ -199,4 +199,5 @@ public function getQuota(); * @since 9.0.0 */ public function setQuota($quota); + } diff --git a/lib/public/User/IProvidesAdditionalSearchAttributesBackend.php b/lib/public/User/IProvidesAdditionalSearchAttributesBackend.php new file mode 100644 index 000000000000..516ec1a1a8b0 --- /dev/null +++ b/lib/public/User/IProvidesAdditionalSearchAttributesBackend.php @@ -0,0 +1,44 @@ + + * + * @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 + * + */ + + +// use OCP namespace for all classes that are considered public. +// This means that they should be used by apps instead of the internal ownCloud classes +namespace OCP\User; + +/** + * Interface IProvidesAdditionalSearchAttributesBackend + * + * @package OCP\User + * @since 10.0.1 + */ +interface IProvidesAdditionalSearchAttributesBackend { + + /** + * Get a users search string for core powered user search + * + * @param string $uid The username + * @return string + * @since 10.0.1 + */ + public function getSearchAttributes($uid); +} + From 9775519f72923e6b9aaf3b67c147259c1f772560 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Thu, 11 May 2017 16:59:22 +0100 Subject: [PATCH 03/30] Add search attribute to user interface --- lib/private/Group/Manager.php | 2 +- lib/private/User/Account.php | 1 + lib/private/User/RemoteUser.php | 13 +++++++++++++ lib/private/User/User.php | 16 ++++++++++++++++ lib/public/IUser.php | 17 +++++++++++++++++ 5 files changed, 48 insertions(+), 1 deletion(-) diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 1eed21471885..48557d2a72f8 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -397,7 +397,7 @@ public function findUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) { foreach($groupUsers as $groupUser) { $matchingUsers[$groupUser->getUID()] = $groupUser; } - die(var_dump($matchingUsers)); + return $matchingUsers; } diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index 92fde74c619d..9d201dffa973 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -46,6 +46,7 @@ * @method string getHome() * @method void setHome(string $home) * @method void setSearchAttributes(string $searchAttributes) + * @method string getSearchAttributes() * * @package OC\User */ diff --git a/lib/private/User/RemoteUser.php b/lib/private/User/RemoteUser.php index 782f321cf60d..8c0d0012e261 100644 --- a/lib/private/User/RemoteUser.php +++ b/lib/private/User/RemoteUser.php @@ -197,4 +197,17 @@ public function getQuota() { public function setQuota($quota) { } + /** + * @inheritdoc + */ + public function getSearchAttributes() { + return ''; + } + + /** + * @inheritdoc + */ + public function setSearchAttributes($searchString) { + } + } diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 55bc09a57ffd..715818b7200e 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -437,4 +437,20 @@ public function triggerChange($feature, $value = null) { $this->emitter->emit('\OC\User', 'changeUser', [$this, $feature, $value]); } } + + /** + * @return string + * @since 10.0.1 + */ + public function getSearchAttributes() { + return $this->account->getSearchAttributes(); + } + + /** + * @return string + * @since 10.0.1 + */ + public function setSearchAttributes($searchString) { + $this->account->setSearchAttributes($searchString); + } } diff --git a/lib/public/IUser.php b/lib/public/IUser.php index 5df691d73384..f8fc418a541f 100644 --- a/lib/public/IUser.php +++ b/lib/public/IUser.php @@ -200,4 +200,21 @@ public function getQuota(); */ public function setQuota($quota); + /** + * set the users' search attributes + * + * @param string $searchString + * @return void + * @since 10.0.1 + */ + public function setSearchAttributes($searchString); + + /** + * get the users' search attributes + * + * @return string + * @since 10.0.1 + */ + public function getSearchAttributes(); + } From ff083732866af5f44e91b99e24025e4f3c171085 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Thu, 11 May 2017 16:59:22 +0100 Subject: [PATCH 04/30] Add search attribute to user interface --- core/Migrations/Version20170510143952.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/Migrations/Version20170510143952.php b/core/Migrations/Version20170510143952.php index 6fcd13883817..8c3e89a1a7eb 100644 --- a/core/Migrations/Version20170510143952.php +++ b/core/Migrations/Version20170510143952.php @@ -36,7 +36,7 @@ public function changeSchema(Schema $schema, array $options) { // Add column to hold additional search attributes $table->addColumn('search_attributes', 'string', [ 'notnull' => false, - 'length' => 64, + 'length' => 185, // Max index length ]); // Add index for search attributes From d5933a37c0f0ed498bb1e1e85472dece880a20ac Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Fri, 12 May 2017 13:37:25 +0100 Subject: [PATCH 05/30] Update sharee unit tests for custom search attribute support --- apps/files_sharing/tests/API/ShareesTest.php | 32 ++++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/apps/files_sharing/tests/API/ShareesTest.php b/apps/files_sharing/tests/API/ShareesTest.php index 450b9b871b20..e69fe158a7db 100644 --- a/apps/files_sharing/tests/API/ShareesTest.php +++ b/apps/files_sharing/tests/API/ShareesTest.php @@ -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, []], ], [], @@ -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, []], ], [], @@ -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'), ]], ], [], @@ -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'), ]], ], [], @@ -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'), ]], ], [ @@ -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'), ]], ], [ @@ -504,7 +504,7 @@ public function testGetUsers( if (!$shareWithGroupOnly && !$shareeEnumerationGroupMembers) { $this->userManager->expects($this->once()) - ->method('searchDisplayName') + ->method('find') ->with($searchTerm, $this->invokePrivate($this->sharees, 'limit'), $this->invokePrivate($this->sharees, 'offset')) ->willReturn($userResponse); } else { @@ -527,7 +527,7 @@ public function testGetUsers( } $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); } From 53fc71e390ec0fc1b76b54abb63a8ecc0f7781d5 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Fri, 12 May 2017 13:43:10 +0100 Subject: [PATCH 06/30] Renamed search attribute backend interface --- lib/private/User/Manager.php | 5 ++--- lib/private/User/SyncService.php | 4 ++-- ...ributesBackend.php => IProvidesExtendedSearchBackend.php} | 5 +++-- 3 files changed, 7 insertions(+), 7 deletions(-) rename lib/public/User/{IProvidesAdditionalSearchAttributesBackend.php => IProvidesExtendedSearchBackend.php} (87%) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 175ccd849ab0..e5aef26cb716 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -38,7 +38,7 @@ use OCP\IUser; use OCP\IUserManager; use OCP\IConfig; -use OCP\User\IProvidesAdditionalSearchAttributesBackend; +use OCP\User\IProvidesExtendedSearchBackend; use OCP\User\IProvidesEMailBackend; use OCP\User\IProvidesQuotaBackend; use OCP\UserInterface; @@ -258,7 +258,6 @@ public function find($pattern, $limit = null, $offset = null) { $user = $this->getUserObject($account); $users[$user->getUID()] = $user; } - return $users; } @@ -435,7 +434,7 @@ private function newAccount($uid, $backend) { $account->setQuota($quota); } } - if ($backend instanceof IProvidesAdditionalSearchAttributesBackend) { + if ($backend instanceof IProvidesExtendedSearchBackend) { $searchString = $backend->getSearchAttributes($uid); if ($searchString !== null) { $account->setSearchAttributes($searchString); diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index fc1c5fcbd37f..d9fac48eae96 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -23,7 +23,7 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\IConfig; use OCP\ILogger; -use OCP\User\IProvidesAdditionalSearchAttributesBackend; +use OCP\User\IProvidesExtendedSearchBackend; use OCP\UserInterface; /** @@ -160,7 +160,7 @@ public function setupAccount(Account $a, $uid) { $a->setDisplayName($this->backend->getDisplayName($uid)); } // Check if backend supplies an additional search string - if ($this->backend instanceof IProvidesAdditionalSearchAttributesBackend) { + if ($this->backend instanceof IProvidesExtendedSearchBackend) { $a->setSearchAttributes($this->backend->getSearchAttributes($uid)); } return $a; diff --git a/lib/public/User/IProvidesAdditionalSearchAttributesBackend.php b/lib/public/User/IProvidesExtendedSearchBackend.php similarity index 87% rename from lib/public/User/IProvidesAdditionalSearchAttributesBackend.php rename to lib/public/User/IProvidesExtendedSearchBackend.php index 516ec1a1a8b0..b38c1bf99366 100644 --- a/lib/public/User/IProvidesAdditionalSearchAttributesBackend.php +++ b/lib/public/User/IProvidesExtendedSearchBackend.php @@ -25,12 +25,13 @@ namespace OCP\User; /** - * Interface IProvidesAdditionalSearchAttributesBackend + * Interface IProvidesExtendedSearchBackend * + * TODO update these backend interface names to be consistent and readable * @package OCP\User * @since 10.0.1 */ -interface IProvidesAdditionalSearchAttributesBackend { +interface IProvidesExtendedSearchBackend { /** * Get a users search string for core powered user search From a73ef473648af005f2cf51f0500e4905c1817b7a Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Fri, 12 May 2017 14:08:44 +0100 Subject: [PATCH 07/30] Add tests for group|user manager find methods --- tests/lib/Group/ManagerTest.php | 335 ++++++++++++++++++++++++++++++++ tests/lib/User/ManagerTest.php | 55 ++++++ 2 files changed, 390 insertions(+) diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index aab23965169c..b4d88c43d788 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -1137,4 +1137,339 @@ public function testGroupDisplayName() { $this->assertEquals('group2', $group->getDisplayName()); } + public function testFindUsersInGroupWithOneUserBackend() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->any()) + ->method('inGroup') + ->will($this->returnCallback(function($uid, $gid) { + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + default: + return null; + } + })); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $userBackend = $this->createMock('\OC_User_Backend'); + + $userManager->expects($this->any()) + ->method('find') + ->with('user3') + ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return []; + } + })); + + $userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) use ($userBackend) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->findUsersInGroup('testgroup', 'user3'); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + + public function testFindUsersInGroupWithOneUserBackendWithLimitSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->any()) + ->method('inGroup') + ->will($this->returnCallback(function($uid, $gid) { + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + case 'user333': return true; + default: + return null; + } + })); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $userBackend = $this->createMock('\OC_User_Backend'); + + $userManager->expects($this->any()) + ->method('find') + ->with('user3') + ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + switch($offset) { + case 0 : return ['user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33')]; + case 2 : return ['user333' => $this->getTestUser('user333')]; + } + })); + + $userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) use ($userBackend) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + case 'user333': return $this->getTestUser('user333'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->findUsersInGroup('testgroup', 'user3', 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + $this->assertFalse(isset($users['user333'])); + } + + public function testFindUsersInGroupWithOneUserBackendWithLimitAndOffsetSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->any()) + ->method('inGroup') + ->will($this->returnCallback(function($uid) { + switch($uid) { + case 'user1' : return false; + case 'user2' : return true; + case 'user3' : return false; + case 'user33': return true; + case 'user333': return true; + default: + return null; + } + })); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $userBackend = $this->createMock('\OC_User_Backend'); + + $userManager->expects($this->any()) + ->method('find') + ->with('user3') + ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + switch($offset) { + case 0 : + return [ + 'user3' => $this->getTestUser('user3'), + 'user33' => $this->getTestUser('user33'), + 'user333' => $this->getTestUser('user333') + ]; + } + })); + + $userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) use ($userBackend) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + case 'user333': return $this->getTestUser('user333'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->findUsersInGroup('testgroup', 'user3', 1, 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertFalse(isset($users['user33'])); + $this->assertTrue(isset($users['user333'])); + } + + public function testFindUsersInGroupWithOneUserBackendAndSearchEmpty() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', -1, 0) + ->will($this->returnValue(['user2', 'user33'])); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $userBackend = $this->createMock('\OC_User_Backend'); + + $userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) use ($userBackend) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->findUsersInGroup('testgroup', ''); + $this->assertEquals(2, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertTrue(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + + public function testFindUsersInGroupWithOneUserBackendAndSearchEmptyAndLimitSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', 1, 0) + ->will($this->returnValue(['user2'])); + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $userBackend = $this->createMock('\OC_User_Backend'); + + $userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) use ($userBackend) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->findUsersInGroup('testgroup', '', 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertTrue(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertFalse(isset($users['user33'])); + } + + public function testFindUsersInGroupWithOneUserBackendAndSearchEmptyAndLimitAndOffsetSpecified() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->getTestBackend(); + $backend->expects($this->exactly(1)) + ->method('groupExists') + ->with('testgroup') + ->will($this->returnValue(true)); + + $backend->expects($this->once()) + ->method('usersInGroup') + ->with('testgroup', '', 1, 1) + ->will($this->returnValue(['user33'])); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $userBackend = $this->createMock('\OC_User_Backend'); + + $userManager->expects($this->any()) + ->method('get') + ->will($this->returnCallback(function($uid) use ($userBackend) { + switch($uid) { + case 'user1' : return $this->getTestUser('user1'); + case 'user2' : return $this->getTestUser('user2'); + case 'user3' : return $this->getTestUser('user3'); + case 'user33': return $this->getTestUser('user33'); + default: + return null; + } + })); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $users = $manager->findUsersInGroup('testgroup', '', 1, 1); + $this->assertEquals(1, count($users)); + $this->assertFalse(isset($users['user1'])); + $this->assertFalse(isset($users['user2'])); + $this->assertFalse(isset($users['user3'])); + $this->assertTrue(isset($users['user33'])); + } + } diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index 9acc4bcebdcf..7c8b18900d62 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -129,6 +129,61 @@ public function testGetOneBackendNotExists() { $this->assertEquals(null, $this->manager->get('foo')); } + public function testFind() { + $a0 = new Account(); + $a0->setUserId('afoo'); + $a1 = new Account(); + $a1->setUserId('foo'); + $this->accountMapper->expects($this->once())->method('find') + ->with('fo')->willReturn([$a0, $a1]); + $result = $this->manager->find('fo'); + $this->assertEquals(2, count($result)); + $this->assertEquals('afoo', array_shift($result)->getUID()); + $this->assertEquals('foo', array_shift($result)->getUID()); + } + + public function testFindWithLimit() { + $a0 = new Account(); + $a0->setUserId('afoo'); + $a1 = new Account(); + $a1->setUserId('foo'); + $this->accountMapper->expects($this->once())->method('find') + ->with('fo', 1)->willReturn([$a0]); + $result = $this->manager->find('fo', 1); + $this->assertEquals(1, count($result)); + $this->assertEquals('afoo', array_shift($result)->getUID()); + } + + public function testFindWithDisplayName() { + $a0 = new Account(); + $a0->setUserId('afoo'); + $a0->setDisplayName('display1'); + $a1 = new Account(); + $a1->setUserId('foo'); + $a0->setDisplayName('display2'); + $this->accountMapper->expects($this->once())->method('find') + ->with('display2')->willReturn([$a1]); + $result = $this->manager->find('display2'); + $this->assertEquals(1, count($result)); + $this->assertEquals('foo', array_shift($result)->getUID()); + } + + public function testFindWithEmail() { + $a0 = new Account(); + $a0->setUserId('afoo'); + $a0->setEmail('test@test.com'); + $a1 = new Account(); + $a1->setUserId('foo'); + $a0->setEmail('test2@test.com'); + $this->accountMapper->expects($this->once())->method('find') + ->with('@test.com')->willReturn([$a0, $a1]); + $result = $this->manager->find('@test.com'); + $this->assertEquals(2, count($result)); + $this->assertEquals('afoo', array_shift($result)->getUID()); + $this->assertEquals('foo', array_shift($result)->getUID()); + } + + public function testSearch() { $a0 = new Account(); $a0->setUserId('afoo'); From 7a881874f01f9c4bf29da9de902a9499fc7f2efd Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Fri, 12 May 2017 15:32:59 +0100 Subject: [PATCH 08/30] Update account after changing search attributes string --- lib/private/User/User.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 715818b7200e..fccceb07cb3e 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -452,5 +452,6 @@ public function getSearchAttributes() { */ public function setSearchAttributes($searchString) { $this->account->setSearchAttributes($searchString); + $this->mapper->update($this->account); } } From 387fd9e30bd3a4c640a56d22d62a159312466425 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Tue, 16 May 2017 12:49:25 +0100 Subject: [PATCH 09/30] WIP account_terms table --- core/Migrations/Version20170510143952.php | 55 -------------- core/Migrations/Version20170516100103.php | 41 ++++++++++ lib/private/User/Account.php | 30 ++++++-- lib/private/User/AccountMapper.php | 75 ++++++++++++++++++- lib/private/User/Manager.php | 6 +- lib/private/User/RemoteUser.php | 6 +- lib/private/User/SyncService.php | 2 +- lib/private/User/User.php | 13 ++-- lib/public/IUser.php | 12 +-- .../User/IProvidesExtendedSearchBackend.php | 6 +- 10 files changed, 162 insertions(+), 84 deletions(-) delete mode 100644 core/Migrations/Version20170510143952.php create mode 100644 core/Migrations/Version20170516100103.php diff --git a/core/Migrations/Version20170510143952.php b/core/Migrations/Version20170510143952.php deleted file mode 100644 index 8c3e89a1a7eb..000000000000 --- a/core/Migrations/Version20170510143952.php +++ /dev/null @@ -1,55 +0,0 @@ - - * - * @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 - * - */ -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'); - - // 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'); - - } -} diff --git a/core/Migrations/Version20170516100103.php b/core/Migrations/Version20170516100103.php new file mode 100644 index 000000000000..d08f7cd7f374 --- /dev/null +++ b/core/Migrations/Version20170516100103.php @@ -0,0 +1,41 @@ +hasTable("${prefix}account_terms")) { + $table = $schema->createTable("${prefix}account_terms"); + + $table->addColumn('id', Type::BIGINT, [ + 'autoincrement' => true, + 'unsigned' => true, + 'notnull' => true, + ]); + + // Foreign key to oc_accounts.id column + $table->addColumn('account_id', Type::BIGINT, [ + 'notnull' => true, + 'unsigned' => true, + ]); + + $table->addColumn('term', Type::STRING, [ + 'notnull' => true, + 'length' => 256 + ]); + + $table->setPrimaryKey(['id']); + $table->addIndex(['account_id'], 'account_id_index'); + $table->addIndex(['term'], 'term_index'); + + } + } +} diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index 9d201dffa973..3e836f74975e 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -22,9 +22,7 @@ namespace OC\User; - use OCP\AppFramework\Db\Entity; -use OCP\AppFramework\QueryException; use OCP\UserInterface; /** @@ -45,8 +43,7 @@ * @method void setQuota(string $quota) * @method string getHome() * @method void setHome(string $home) - * @method void setSearchAttributes(string $searchAttributes) - * @method string getSearchAttributes() + * @method string[] getSearchTerms() * * @package OC\User */ @@ -66,7 +63,9 @@ class Account extends Entity { protected $backend; protected $state; protected $home; - protected $searchAttributes; + + protected $terms = []; + private $_termsChanged = false; public function __construct() { $this->addType('state', 'integer'); @@ -89,4 +88,25 @@ public function getBackendInstance() { // actually stupid return \OC::$server->getUserManager()->getBackend($backendClass); } + + public function getUpdatedFields() { + $fields = parent::getUpdatedFields(); + unset($fields['terms']); + return $fields; + } + + public function haveTermsAltered() { + return $this->_termsChanged; + } + + /** + * @param string[] $terms + */ + public function setSearchTerms(array $terms) { + if(array_diff($terms, $this->terms)) { + $this->terms = $terms; + $this->_termsChanged = true; + } + } + } diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 897c1040f9f6..0aa5693fd16d 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -24,6 +24,7 @@ use OC\DB\QueryBuilder\Literal; +use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\Mapper; use OCP\IDBConnection; @@ -33,6 +34,28 @@ public function __construct(IDBConnection $db) { parent::__construct($db, 'accounts', Account::class); } + public function insert(Entity $entity) { + if($entity->haveTermsChanged()) { + $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); + } + // Then run the normal entity insert operation + parent::insert($entity); + } + + public function delete(Entity $entity) { + // First delete the search terms for this account + $this->deleteTermsForAccount($entity->getId()); + parent::delete($entity); + } + + public function update(Entity $entity) { + if($entity->haveTermsChanged()) { + $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); + } + // Then run the normal entity insert operation + parent::update($entity); + } + /** * @param string $email * @return Account[] @@ -89,10 +112,11 @@ public function find($pattern, $limit, $offset) { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) + ->join('account', 'account_terms', 'term', $qb->expr()->eq('a.id', 'term.account_id')) ->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) . '%'))) ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->iLike('search_attributes', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('term.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orderBy('display_name'); return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset); @@ -159,4 +183,53 @@ public function callForAllUsers($callback, $search, $onlySeen) { $stmt->closeCursor(); } + /** + * Sets search terms for a given account id + * @param $account_id + * @param string[] $terms + */ + public function setTermsForAccount($account_id, array $terms) { + // Delete all terms for this account + $this->deleteTermsForAccount($account_id); + // Now batch insert the new terms for this account + $qb = $this->db->getQueryBuilder(); + $terms = array_map(function($term) use ($account_id) { + return ['account_id' => $account_id, 'term' => $term]; + }, $terms); + $qb + ->insert('account_terms') + ->values($terms) + ->execute(); + } + + /** + * Removes all search terms for a given account id + * @param $account_id + */ + public function deleteTermsForAccount($account_id) { + $qb = $this->db->getQueryBuilder(); + $qb + ->delete('account_terms') + ->where($qb->expr()->eq('account_id', $qb->createNamedParameter($account_id))) + ->execute(); + } + + /** + * Retrieves all search terms attached to an account id + * @param $account_id + * @return array of account search terms + */ + public function getTermsForAccount($account_id) { + $qb = $this->db->getQueryBuilder(); + $results = $qb + ->select('term') + ->from('account_terms') + ->where($qb->expr()->eq('account_id', $qb->createNamedParameter($account_id))) + ->execute(); + $terms = []; + while($row = $results->fetch()) { + $terms[] = $row['term']; + } + return $terms; + } } diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index e5aef26cb716..a33dd33bd39e 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -435,9 +435,9 @@ private function newAccount($uid, $backend) { } } if ($backend instanceof IProvidesExtendedSearchBackend) { - $searchString = $backend->getSearchAttributes($uid); - if ($searchString !== null) { - $account->setSearchAttributes($searchString); + $terms = $backend->getSearchTerms($uid); + if (!empty($terms)) { + $account->setSearchTerms($terms); } } $home = false; diff --git a/lib/private/User/RemoteUser.php b/lib/private/User/RemoteUser.php index 8c0d0012e261..cae025f7bdff 100644 --- a/lib/private/User/RemoteUser.php +++ b/lib/private/User/RemoteUser.php @@ -200,14 +200,14 @@ public function setQuota($quota) { /** * @inheritdoc */ - public function getSearchAttributes() { - return ''; + public function getSearchTerms() { + return []; } /** * @inheritdoc */ - public function setSearchAttributes($searchString) { + public function setSearchTerms(array $terms) { } } diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index d9fac48eae96..604f0a5bff77 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -161,7 +161,7 @@ public function setupAccount(Account $a, $uid) { } // Check if backend supplies an additional search string if ($this->backend instanceof IProvidesExtendedSearchBackend) { - $a->setSearchAttributes($this->backend->getSearchAttributes($uid)); + $a->setSearchTerms($this->backend->getSearchTerms($uid)); } return $a; } diff --git a/lib/private/User/User.php b/lib/private/User/User.php index fccceb07cb3e..1edb851b39c3 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -439,19 +439,18 @@ public function triggerChange($feature, $value = null) { } /** - * @return string + * @return array * @since 10.0.1 */ - public function getSearchAttributes() { - return $this->account->getSearchAttributes(); + public function getSearchTerms() { + return $this->account->getSearchTerms(); } /** - * @return string + * @param array $terms * @since 10.0.1 */ - public function setSearchAttributes($searchString) { - $this->account->setSearchAttributes($searchString); - $this->mapper->update($this->account); + public function setSearchTerms(array $terms) { + $this->account->setSearchTerms($terms); } } diff --git a/lib/public/IUser.php b/lib/public/IUser.php index f8fc418a541f..08581f0767d3 100644 --- a/lib/public/IUser.php +++ b/lib/public/IUser.php @@ -201,20 +201,20 @@ public function getQuota(); public function setQuota($quota); /** - * set the users' search attributes + * set the users' search terms * - * @param string $searchString + * @param array $terms * @return void * @since 10.0.1 */ - public function setSearchAttributes($searchString); + public function setSearchTerms(array $terms); /** - * get the users' search attributes + * get the users' search terms * - * @return string + * @return array * @since 10.0.1 */ - public function getSearchAttributes(); + public function getSearchTerms(); } diff --git a/lib/public/User/IProvidesExtendedSearchBackend.php b/lib/public/User/IProvidesExtendedSearchBackend.php index b38c1bf99366..be67b39724ff 100644 --- a/lib/public/User/IProvidesExtendedSearchBackend.php +++ b/lib/public/User/IProvidesExtendedSearchBackend.php @@ -34,12 +34,12 @@ interface IProvidesExtendedSearchBackend { /** - * Get a users search string for core powered user search + * Get search terms for a users account for core powered user search * * @param string $uid The username - * @return string + * @return array * @since 10.0.1 */ - public function getSearchAttributes($uid); + public function getSearchTerms($uid); } From 360753a4d00c4ed9059d0978c21020216e4ee4e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 16 May 2017 14:18:28 +0200 Subject: [PATCH 10/30] minor fixes --- lib/private/User/Account.php | 2 +- lib/private/User/AccountMapper.php | 15 ++++++++++++--- .../User/IProvidesExtendedSearchBackend.php | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index 3e836f74975e..c2e138464ec8 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -95,7 +95,7 @@ public function getUpdatedFields() { return $fields; } - public function haveTermsAltered() { + public function haveTermsChanged() { return $this->_termsChanged; } diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 0aa5693fd16d..9be189e70d29 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -34,6 +34,9 @@ public function __construct(IDBConnection $db) { parent::__construct($db, 'accounts', Account::class); } + /** + * @param Account $entity + */ public function insert(Entity $entity) { if($entity->haveTermsChanged()) { $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); @@ -41,13 +44,19 @@ public function insert(Entity $entity) { // Then run the normal entity insert operation parent::insert($entity); } - + + /** + * @param Account $entity + */ public function delete(Entity $entity) { // First delete the search terms for this account $this->deleteTermsForAccount($entity->getId()); parent::delete($entity); } + /** + * @param Account $entity + */ public function update(Entity $entity) { if($entity->haveTermsChanged()) { $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); @@ -112,11 +121,11 @@ public function find($pattern, $limit, $offset) { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) - ->join('account', 'account_terms', 'term', $qb->expr()->eq('a.id', 'term.account_id')) + ->join('account', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) ->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) . '%'))) ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->iLike('term.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('t.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orderBy('display_name'); return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset); diff --git a/lib/public/User/IProvidesExtendedSearchBackend.php b/lib/public/User/IProvidesExtendedSearchBackend.php index be67b39724ff..c23c3a194e1e 100644 --- a/lib/public/User/IProvidesExtendedSearchBackend.php +++ b/lib/public/User/IProvidesExtendedSearchBackend.php @@ -37,7 +37,7 @@ interface IProvidesExtendedSearchBackend { * Get search terms for a users account for core powered user search * * @param string $uid The username - * @return array + * @return string[] * @since 10.0.1 */ public function getSearchTerms($uid); From 8ca31fe0e17416173f91020d09d98b3d9034302c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 16 May 2017 16:05:07 +0200 Subject: [PATCH 11/30] use dedicated AccountTerm Mapper and Entity, use terms during search --- lib/private/User/Account.php | 4 ++ lib/private/User/AccountMapper.php | 61 +++----------------- lib/private/User/AccountTerm.php | 44 +++++++++++++++ lib/private/User/AccountTermMapper.php | 77 ++++++++++++++++++++++++++ lib/private/User/User.php | 18 ++++-- 5 files changed, 145 insertions(+), 59 deletions(-) create mode 100644 lib/private/User/AccountTerm.php create mode 100644 lib/private/User/AccountTermMapper.php diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index c2e138464ec8..1b6687a0d744 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -109,4 +109,8 @@ public function setSearchTerms(array $terms) { } } + public static function fromRow(array $row) { + return parent::fromRow($row); // TODO: Change the autogenerated stub + } + } diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 9be189e70d29..d0470ef51e24 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -119,13 +119,15 @@ public function search($fieldName, $pattern, $limit, $offset) { */ public function find($pattern, $limit, $offset) { $qb = $this->db->getQueryBuilder(); - $qb->select('*') - ->from($this->getTableName()) - ->join('account', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) - ->where($qb->expr()->Like('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter(strtolower($pattern)) . '%'))) + $lowerPattern = strtolower($pattern); + $qb->select(['user_id', 'lower_user_id', 'display_name', 'email', 'last_login', 'backend', 'state', 'quota', 'home']) + ->selectAlias('a.id', 'id') + ->from($this->getTableName(), 'a') + ->join('a', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) + ->where($qb->expr()->Like('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->iLike('t.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) ->orderBy('display_name'); return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset); @@ -192,53 +194,4 @@ public function callForAllUsers($callback, $search, $onlySeen) { $stmt->closeCursor(); } - /** - * Sets search terms for a given account id - * @param $account_id - * @param string[] $terms - */ - public function setTermsForAccount($account_id, array $terms) { - // Delete all terms for this account - $this->deleteTermsForAccount($account_id); - // Now batch insert the new terms for this account - $qb = $this->db->getQueryBuilder(); - $terms = array_map(function($term) use ($account_id) { - return ['account_id' => $account_id, 'term' => $term]; - }, $terms); - $qb - ->insert('account_terms') - ->values($terms) - ->execute(); - } - - /** - * Removes all search terms for a given account id - * @param $account_id - */ - public function deleteTermsForAccount($account_id) { - $qb = $this->db->getQueryBuilder(); - $qb - ->delete('account_terms') - ->where($qb->expr()->eq('account_id', $qb->createNamedParameter($account_id))) - ->execute(); - } - - /** - * Retrieves all search terms attached to an account id - * @param $account_id - * @return array of account search terms - */ - public function getTermsForAccount($account_id) { - $qb = $this->db->getQueryBuilder(); - $results = $qb - ->select('term') - ->from('account_terms') - ->where($qb->expr()->eq('account_id', $qb->createNamedParameter($account_id))) - ->execute(); - $terms = []; - while($row = $results->fetch()) { - $terms[] = $row['term']; - } - return $terms; - } } diff --git a/lib/private/User/AccountTerm.php b/lib/private/User/AccountTerm.php new file mode 100644 index 000000000000..d13a8d556fa3 --- /dev/null +++ b/lib/private/User/AccountTerm.php @@ -0,0 +1,44 @@ + + * + * @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 + * + */ + +namespace OC\User; + + +use OCP\AppFramework\Db\Entity; + +/** + * Class Account + * + * @method int getAccountId() + * @method void setAccountId(int $accountId) + * @method string getTerm() + * @method void setTerm(string $term) + * + * @package OC\User + */ +class AccountTerm extends Entity { + + protected $accountId; + protected $term; + public function __construct() { + $this->addType('accountId', 'integer'); + } +} \ No newline at end of file diff --git a/lib/private/User/AccountTermMapper.php b/lib/private/User/AccountTermMapper.php new file mode 100644 index 000000000000..d79425260cc9 --- /dev/null +++ b/lib/private/User/AccountTermMapper.php @@ -0,0 +1,77 @@ + + * + * @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 + * + */ + +namespace OC\User; + + +use OCP\AppFramework\Db\Mapper; +use OCP\IDBConnection; + +class AccountTermMapper extends Mapper { + + public function __construct(IDBConnection $db) { + parent::__construct($db, 'account_terms', AccountTerm::class); + } + /** + * Sets search terms for a given account id, will always be lowercased + * @param $account_id + * @param string[] $terms + */ + public function setTermsForAccount($account_id, array $terms) { + // Delete all terms for this account + $this->deleteTermsForAccount($account_id); + // Now batch insert the new terms for this account + $qb = $this->db->getQueryBuilder(); + $terms = array_map(function($term) use ($account_id) { + return ['account_id' => $account_id, 'term' => strtolower($term)]; + }, $terms); + $qb + ->insert($this->getTableName()) + ->values($terms) + ->execute(); + } + + /** + * Removes all search terms for a given account id + * @param $account_id + */ + public function deleteTermsForAccount($account_id) { + $qb = $this->db->getQueryBuilder(); + $qb + ->delete($this->getTableName()) + ->where($qb->expr()->eq('account_id', $qb->createNamedParameter($account_id))) + ->execute(); + } + + /** + * Retrieves all search terms attached to an account id + * @param $account_id + * @return AccountTerm[] of account search terms + */ + public function findByAccountId($account_id) { + $qb = $this->db->getQueryBuilder(); + $qb->select('term') + ->from($this->getTableName()) + ->where($qb->expr()->eq('account_id', $qb->createNamedParameter($account_id))); + + return $this->findEntities($qb->getSQL(), $qb->getParameters()); + } +} \ No newline at end of file diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 1edb851b39c3..01dec118adc8 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -66,6 +66,9 @@ class User implements IUser { /** @var AccountMapper */ private $mapper; + /** @var AccountTermMapper */ + private $termMapper; + /** * User constructor. * @@ -76,11 +79,12 @@ class User implements IUser { * @param null $urlGenerator * @param EventDispatcher|null $eventDispatcher */ - public function __construct(Account $account, AccountMapper $mapper, $emitter = null, IConfig $config = null, + public function __construct(Account $account, AccountMapper $mapper, AccountTermMapper $termMapper, $emitter = null, IConfig $config = null, $urlGenerator = null, EventDispatcher $eventDispatcher = null ) { $this->account = $account; $this->mapper = $mapper; + $this->termMapper = $termMapper; $this->emitter = $emitter; $this->eventDispatcher = $eventDispatcher; if(is_null($config)) { @@ -439,18 +443,22 @@ public function triggerChange($feature, $value = null) { } /** - * @return array + * @return string[] * @since 10.0.1 */ public function getSearchTerms() { - return $this->account->getSearchTerms(); + $terms = []; + foreach ($this->termMapper->findByAccountId($this->account->getId()) as $term) { + $terms[] = $term->getTerm(); + } + return $terms; } /** - * @param array $terms + * @param string[] $terms * @since 10.0.1 */ public function setSearchTerms(array $terms) { - $this->account->setSearchTerms($terms); + $this->termMapper->setTermsForAccount($this->account->getId(), $terms); } } From 33327895d494be271d593448663e6da9d1ac3998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 16 May 2017 16:16:40 +0200 Subject: [PATCH 12/30] fix di --- lib/private/User/Manager.php | 8 ++++++-- lib/private/User/User.php | 1 + tests/lib/Files/Config/UserMountCacheTest.php | 7 ++++++- tests/lib/User/ManagerTest.php | 6 +++++- tests/lib/User/UserTest.php | 19 ++++++++++++------- 5 files changed, 30 insertions(+), 11 deletions(-) diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index a33dd33bd39e..ed3ca16dca86 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -74,15 +74,19 @@ class Manager extends PublicEmitter implements IUserManager { /** @var AccountMapper */ private $accountMapper; + /** @var AccountTermMapper */ + private $accountTermMapper; + /** * @param IConfig $config * @param ILogger $logger * @param AccountMapper $accountMapper */ - public function __construct(IConfig $config, ILogger $logger, AccountMapper $accountMapper) { + public function __construct(IConfig $config, ILogger $logger, AccountMapper $accountMapper, AccountTermMapper $accountTermMapper) { $this->config = $config; $this->logger = $logger; $this->accountMapper = $accountMapper; + $this->accountTermMapper = $accountTermMapper; $cachedUsers = &$this->cachedUsers; $this->listen('\OC\User', 'postDelete', function ($user) use (&$cachedUsers) { /** @var \OC\User\User $user */ @@ -173,7 +177,7 @@ protected function getUserObject(Account $account, $cacheUser = true) { return $this->cachedUsers[$account->getUserId()]; } - $user = new User($account, $this->accountMapper, $this, $this->config, null, \OC::$server->getEventDispatcher() ); + $user = new User($account, $this->accountMapper, $this->accountTermMapper, $this, $this->config, null, \OC::$server->getEventDispatcher() ); if ($cacheUser) { $this->cachedUsers[$account->getUserId()] = $user; } diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 01dec118adc8..7baed453439b 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -74,6 +74,7 @@ class User implements IUser { * * @param Account $account * @param AccountMapper $mapper + * @param AccountTermMapper $termMapper * @param null $emitter * @param IConfig|null $config * @param null $urlGenerator diff --git a/tests/lib/Files/Config/UserMountCacheTest.php b/tests/lib/Files/Config/UserMountCacheTest.php index 43a603e005f0..da753cd669a1 100644 --- a/tests/lib/Files/Config/UserMountCacheTest.php +++ b/tests/lib/Files/Config/UserMountCacheTest.php @@ -14,6 +14,7 @@ use OC\Log; use OC\User\Account; use OC\User\AccountMapper; +use OC\User\AccountTermMapper; use OC\User\Manager; use OCP\Files\Config\ICachedMountInfo; use OCP\IConfig; @@ -65,9 +66,13 @@ public function setUp() { ['u2', $a2], ['u3', $a3], ]); + + /** @var AccountTermMapper | \PHPUnit_Framework_MockObject_MockObject $accountTermMapper */ + $accountTermMapper = $this->createMock(AccountTermMapper::class); + /** @var Log $log */ $log = $this->createMock(Log::class); - $this->userManager = new Manager($config, $log, $accountMapper); + $this->userManager = new Manager($config, $log, $accountMapper, $accountTermMapper); $this->cache = new UserMountCache($this->connection, $this->userManager, $log); // hookup listener diff --git a/tests/lib/User/ManagerTest.php b/tests/lib/User/ManagerTest.php index 7c8b18900d62..e85ff504afbf 100644 --- a/tests/lib/User/ManagerTest.php +++ b/tests/lib/User/ManagerTest.php @@ -10,6 +10,7 @@ namespace Test\User; use OC\User\Account; use OC\User\AccountMapper; +use OC\User\AccountTermMapper; use OC\User\Backend; use OC\User\Database; use OC\User\Manager; @@ -32,6 +33,8 @@ class ManagerTest extends TestCase { private $manager; /** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject */ private $accountMapper; + /** @var AccountTermMapper | \PHPUnit_Framework_MockObject_MockObject */ + private $accountTermMapper; public function setUp() { parent::setUp(); @@ -41,7 +44,8 @@ public function setUp() { /** @var ILogger | \PHPUnit_Framework_MockObject_MockObject $logger */ $logger = $this->createMock(ILogger::class); $this->accountMapper = $this->createMock(AccountMapper::class); - $this->manager = new \OC\User\Manager($config, $logger, $this->accountMapper); + $this->accountTermMapper = $this->createMock(AccountTermMapper::class); + $this->manager = new \OC\User\Manager($config, $logger, $this->accountMapper, $this->accountTermMapper); } public function testGetBackends() { diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index 93d068af1073..643f7f6f8b0c 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -12,6 +12,8 @@ use OC\Hooks\PublicEmitter; use OC\User\Account; use OC\User\AccountMapper; +use OC\User\AccountTerm; +use OC\User\AccountTermMapper; use OC\User\Backend; use OC\User\Database; use OC\User\User; @@ -32,6 +34,8 @@ class UserTest extends TestCase { /** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject */ private $accountMapper; + /** @var AccountTermMapper | \PHPUnit_Framework_MockObject_MockObject */ + private $accountTermMapper; /** @var Account */ private $account; /** @var User */ @@ -48,6 +52,7 @@ class UserTest extends TestCase { public function setUp() { parent::setUp(); $this->accountMapper = $this->createMock(AccountMapper::class); + $this->accountTermMapper = $this->createMock(AccountTermMapper::class); $this->config = $this->createMock(IConfig::class); $this->account = new Account(); $this->account->setUserId('foo'); @@ -58,7 +63,7 @@ public function setUp() { ->disableOriginalConstructor() ->getMock(); - $this->user = new User($this->account, $this->accountMapper, $this->emitter, $this->config, $this->urlGenerator, $this->eventDispatcher); + $this->user = new User($this->account, $this->accountMapper, $this->accountTermMapper, $this->emitter, $this->config, $this->urlGenerator, $this->eventDispatcher); } public function testDisplayName() { @@ -85,7 +90,7 @@ public function testSetPassword() { $account->expects($this->any())->method('__call')->with('getUserId')->willReturn('foo'); $backend->expects($this->once())->method('setPassword')->with('foo', 'bar')->willReturn(true); - $this->user = new User($account, $this->accountMapper, null, $this->config); + $this->user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); $this->assertTrue($this->user->setPassword('bar','')); $this->assertTrue($this->user->canChangePassword()); @@ -102,7 +107,7 @@ public function testSetPasswordNotSupported() { $account->expects($this->any())->method('__call')->with('getUserId')->willReturn('foo'); $backend->expects($this->once())->method('setPassword')->with('foo', 'bar')->willReturn(false); - $this->user = new User($account, $this->accountMapper, null, $this->config); + $this->user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); $this->assertFalse($this->user->setPassword('bar','')); $this->assertTrue($this->user->canChangePassword()); } @@ -144,7 +149,7 @@ public function testChangeAvatarSupported($expected, $implements, $canChange) { } })); - $user = new User($account, $this->accountMapper, null, $this->config); + $user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); $this->assertEquals($expected, $user->canChangeAvatar()); } @@ -196,7 +201,7 @@ public function testCanChangeDisplayName($expected, $implements) { } })); - $user = new User($account, $this->accountMapper, null, $this->config); + $user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); $this->assertEquals($expected, $user->canChangeDisplayName()); if ($expected) { @@ -232,7 +237,7 @@ public function testSetDisplayNameNotSupported() { return false; })); - $user = new User($account, $this->accountMapper, null, $this->config); + $user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); $this->assertFalse($user->setDisplayName('Foo')); $this->assertEquals('foo',$user->getDisplayName()); } @@ -262,7 +267,7 @@ public function testSetPasswordHooks() { $account->expects($this->any())->method('__call')->with('getUserId')->willReturn('foo'); $backend->expects($this->once())->method('setPassword')->with('foo', 'bar')->willReturn(true); - $this->user = new User($account, $this->accountMapper, $emitter, $this->config); + $this->user = new User($account, $this->accountMapper, $this->accountTermMapper, $emitter, $this->config); $this->user->setPassword('bar',''); $this->assertEquals(2, $hooksCalled); From a73f2f15fd30a06a7cb6d5032c8a9fb6f48ccfc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 16 May 2017 16:21:13 +0200 Subject: [PATCH 13/30] fix fix for di --- lib/private/Server.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/private/Server.php b/lib/private/Server.php index a893f128924e..5376399e5792 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -86,6 +86,7 @@ use OC\Tagging\TagMapper; use OC\Theme\ThemeService; use OC\User\AccountMapper; +use OC\User\AccountTermMapper; use OCP\IL10N; use OCP\IServerContainer; use OCP\ISession; @@ -224,7 +225,8 @@ public function __construct($webRoot, \OC\Config $config) { $config = $c->getConfig(); $logger = $c->getLogger(); $accountMapper = new AccountMapper($c->getDatabaseConnection()); - return new \OC\User\Manager($config, $logger, $accountMapper); + $accountTermMapper = new AccountTermMapper($c->getDatabaseConnection()); + return new \OC\User\Manager($config, $logger, $accountMapper, $accountTermMapper); }); $this->registerService('GroupManager', function (Server $c) { $groupManager = new \OC\Group\Manager($this->getUserManager()); From c9164821058a3ccbcf7352e45178e935817a4bf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 16 May 2017 16:30:59 +0200 Subject: [PATCH 14/30] return entity en insert, update and delete --- lib/private/User/AccountMapper.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index d0470ef51e24..a12760581139 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -36,33 +36,36 @@ public function __construct(IDBConnection $db) { /** * @param Account $entity + * @return Entity the saved entity with the set id */ public function insert(Entity $entity) { if($entity->haveTermsChanged()) { $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); } // Then run the normal entity insert operation - parent::insert($entity); + return parent::insert($entity); } /** * @param Account $entity + * @return Entity the deleted entity */ public function delete(Entity $entity) { // First delete the search terms for this account $this->deleteTermsForAccount($entity->getId()); - parent::delete($entity); + return parent::delete($entity); } /** * @param Account $entity + * @return Entity the saved entity with the set id */ public function update(Entity $entity) { if($entity->haveTermsChanged()) { $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); } // Then run the normal entity insert operation - parent::update($entity); + return parent::update($entity); } /** From 8dbeb5415bbffa4cb69121ed4c2be5d1031127b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Tue, 16 May 2017 17:23:08 +0200 Subject: [PATCH 15/30] fix insert --- lib/private/User/AccountTermMapper.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/private/User/AccountTermMapper.php b/lib/private/User/AccountTermMapper.php index d79425260cc9..c05763020ee9 100644 --- a/lib/private/User/AccountTermMapper.php +++ b/lib/private/User/AccountTermMapper.php @@ -40,13 +40,15 @@ public function setTermsForAccount($account_id, array $terms) { $this->deleteTermsForAccount($account_id); // Now batch insert the new terms for this account $qb = $this->db->getQueryBuilder(); - $terms = array_map(function($term) use ($account_id) { - return ['account_id' => $account_id, 'term' => strtolower($term)]; - }, $terms); - $qb - ->insert($this->getTableName()) - ->values($terms) - ->execute(); + $qb->insert($this->getTableName()) + ->setValue('account_id', $qb->createParameter('account_id')) + ->setValue('term', $qb->createParameter('term')) + ->setParameter('account_id', $account_id); + + foreach ($terms as $term) { + $qb->setParameter('term', strtolower($term)); + $qb->execute(); + } } /** From 2f1468c7d7a2e278ec19f3bbcee261f111779e32 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Tue, 16 May 2017 17:29:16 +0100 Subject: [PATCH 16/30] Inject account term mapper into account mapper and update tests --- core/Migrations/Version20170221114437.php | 3 ++- lib/private/Server.php | 2 +- lib/private/User/AccountMapper.php | 12 ++++++++---- tests/lib/Traits/UserTrait.php | 3 ++- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/core/Migrations/Version20170221114437.php b/core/Migrations/Version20170221114437.php index 606fba757994..ce4183628d67 100644 --- a/core/Migrations/Version20170221114437.php +++ b/core/Migrations/Version20170221114437.php @@ -3,6 +3,7 @@ use OC\User\Account; use OC\User\AccountMapper; +use OC\User\AccountTermMapper; use OC\User\Database; use OC\User\SyncService; use OCP\IConfig; @@ -16,7 +17,7 @@ class Version20170221114437 implements ISimpleMigration { */ public function run(IOutput $out) { $backend = new Database(); - $accountMapper = new AccountMapper(\OC::$server->getDatabaseConnection()); + $accountMapper = new AccountMapper(\OC::$server->getDatabaseConnection(), new AccountTermMapper(\OC::$server->getDatabaseConnection())); $config = \OC::$server->getConfig(); $logger = \OC::$server->getLogger(); $syncService = new SyncService($accountMapper, $backend, $config, $logger); diff --git a/lib/private/Server.php b/lib/private/Server.php index 5376399e5792..91162d5eedf0 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -224,8 +224,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerService('UserManager', function (Server $c) { $config = $c->getConfig(); $logger = $c->getLogger(); - $accountMapper = new AccountMapper($c->getDatabaseConnection()); $accountTermMapper = new AccountTermMapper($c->getDatabaseConnection()); + $accountMapper = new AccountMapper($c->getDatabaseConnection(), $accountTermMapper); return new \OC\User\Manager($config, $logger, $accountMapper, $accountTermMapper); }); $this->registerService('GroupManager', function (Server $c) { diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index a12760581139..11bbc678a584 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -30,8 +30,12 @@ class AccountMapper extends Mapper { - public function __construct(IDBConnection $db) { + /* @var AccountTermMapper */ + protected $termMapper; + + public function __construct(IDBConnection $db, AccountTermMapper $termMapper) { parent::__construct($db, 'accounts', Account::class); + $this->termMapper = $termMapper; } /** @@ -40,7 +44,7 @@ public function __construct(IDBConnection $db) { */ public function insert(Entity $entity) { if($entity->haveTermsChanged()) { - $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); + $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); } // Then run the normal entity insert operation return parent::insert($entity); @@ -52,7 +56,7 @@ public function insert(Entity $entity) { */ public function delete(Entity $entity) { // First delete the search terms for this account - $this->deleteTermsForAccount($entity->getId()); + $this->termMapper->deleteTermsForAccount($entity->getId()); return parent::delete($entity); } @@ -62,7 +66,7 @@ public function delete(Entity $entity) { */ public function update(Entity $entity) { if($entity->haveTermsChanged()) { - $this->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); + $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); } // Then run the normal entity insert operation return parent::update($entity); diff --git a/tests/lib/Traits/UserTrait.php b/tests/lib/Traits/UserTrait.php index b4a11f43858f..171de3a32c7c 100644 --- a/tests/lib/Traits/UserTrait.php +++ b/tests/lib/Traits/UserTrait.php @@ -8,6 +8,7 @@ namespace Test\Traits; +use OC\User\AccountTermMapper; use OC\User\User; use Test\Util\User\Dummy; use Test\Util\User\MemoryAccountMapper; @@ -38,7 +39,7 @@ protected function createUser($name, $password = null) { protected function setUpUserTrait() { $db = \OC::$server->getDatabaseConnection(); - $accountMapper = new MemoryAccountMapper($db); + $accountMapper = new MemoryAccountMapper($db, new AccountTermMapper($db)); $accountMapper->testCaseName = get_class($this); $this->previousUserManagerInternals = \OC::$server->getUserManager() ->reset($accountMapper, [Dummy::class => new Dummy()]); From dd377c94b645bc50d3e365ec500aafa72f507002 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Wed, 17 May 2017 10:10:07 +0100 Subject: [PATCH 17/30] Update tests and clean code --- apps/files_sharing/tests/API/ShareesTest.php | 2 +- lib/private/Server.php | 2 +- lib/private/User/Account.php | 4 - lib/private/User/AccountMapper.php | 18 +++ lib/private/User/AccountTermMapper.php | 12 +- lib/private/User/Manager.php | 8 +- lib/private/User/User.php | 11 +- lib/public/IGroupManager.php | 2 +- tests/lib/Files/Config/UserMountCacheTest.php | 7 +- tests/lib/Group/ManagerTest.php | 114 +++++++++--------- tests/lib/User/UserTest.php | 19 ++- 11 files changed, 96 insertions(+), 103 deletions(-) diff --git a/apps/files_sharing/tests/API/ShareesTest.php b/apps/files_sharing/tests/API/ShareesTest.php index e69fe158a7db..4a1c70bd1867 100644 --- a/apps/files_sharing/tests/API/ShareesTest.php +++ b/apps/files_sharing/tests/API/ShareesTest.php @@ -424,7 +424,7 @@ public function dataGetUsers() { // args and user response for "displayNamesInGroup" call [ ['group1', 'ano', 2, 0, [ - 'another1' => 'Another One', + 'another1' => $this->getUserMock('another1', 'Another One'), ]], ['group2', 'ano', 2, 0, [ ]], diff --git a/lib/private/Server.php b/lib/private/Server.php index 91162d5eedf0..7e3d0e973a6f 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -226,7 +226,7 @@ public function __construct($webRoot, \OC\Config $config) { $logger = $c->getLogger(); $accountTermMapper = new AccountTermMapper($c->getDatabaseConnection()); $accountMapper = new AccountMapper($c->getDatabaseConnection(), $accountTermMapper); - return new \OC\User\Manager($config, $logger, $accountMapper, $accountTermMapper); + return new \OC\User\Manager($config, $logger, $accountMapper); }); $this->registerService('GroupManager', function (Server $c) { $groupManager = new \OC\Group\Manager($this->getUserManager()); diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index 1b6687a0d744..c2e138464ec8 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -109,8 +109,4 @@ public function setSearchTerms(array $terms) { } } - public static function fromRow(array $row) { - return parent::fromRow($row); // TODO: Change the autogenerated stub - } - } diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 11bbc678a584..ac24767df4fe 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -38,6 +38,24 @@ public function __construct(IDBConnection $db, AccountTermMapper $termMapper) { $this->termMapper = $termMapper; } + /** + * Pass through call to term mapper to avoid needing to inject term mapper + * @param $account_id + * @param array $terms + */ + public function setTermsForAccount($account_id, array $terms) { + $this->termMapper->setTermsForAccount($account_id, $terms); + } + + /** + * Pass through call to term mapper to avoid needing to inject term mapper + * @param $account_id + * @return AccountTerm[] $terms + */ + public function findByAccountId($account_id) { + return $this->termMapper->findByAccountId($account_id); + } + /** * @param Account $entity * @return Entity the saved entity with the set id diff --git a/lib/private/User/AccountTermMapper.php b/lib/private/User/AccountTermMapper.php index c05763020ee9..f1e3ee6a8797 100644 --- a/lib/private/User/AccountTermMapper.php +++ b/lib/private/User/AccountTermMapper.php @@ -39,15 +39,11 @@ public function setTermsForAccount($account_id, array $terms) { // Delete all terms for this account $this->deleteTermsForAccount($account_id); // Now batch insert the new terms for this account - $qb = $this->db->getQueryBuilder(); - $qb->insert($this->getTableName()) - ->setValue('account_id', $qb->createParameter('account_id')) - ->setValue('term', $qb->createParameter('term')) - ->setParameter('account_id', $account_id); - foreach ($terms as $term) { - $qb->setParameter('term', strtolower($term)); - $qb->execute(); + $t = new AccountTerm(); + $t->setAccountId($account_id); + $t->setTerm(strtolower($term)); + $this->insert($t); } } diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index ed3ca16dca86..a33dd33bd39e 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -74,19 +74,15 @@ class Manager extends PublicEmitter implements IUserManager { /** @var AccountMapper */ private $accountMapper; - /** @var AccountTermMapper */ - private $accountTermMapper; - /** * @param IConfig $config * @param ILogger $logger * @param AccountMapper $accountMapper */ - public function __construct(IConfig $config, ILogger $logger, AccountMapper $accountMapper, AccountTermMapper $accountTermMapper) { + public function __construct(IConfig $config, ILogger $logger, AccountMapper $accountMapper) { $this->config = $config; $this->logger = $logger; $this->accountMapper = $accountMapper; - $this->accountTermMapper = $accountTermMapper; $cachedUsers = &$this->cachedUsers; $this->listen('\OC\User', 'postDelete', function ($user) use (&$cachedUsers) { /** @var \OC\User\User $user */ @@ -177,7 +173,7 @@ protected function getUserObject(Account $account, $cacheUser = true) { return $this->cachedUsers[$account->getUserId()]; } - $user = new User($account, $this->accountMapper, $this->accountTermMapper, $this, $this->config, null, \OC::$server->getEventDispatcher() ); + $user = new User($account, $this->accountMapper, $this, $this->config, null, \OC::$server->getEventDispatcher() ); if ($cacheUser) { $this->cachedUsers[$account->getUserId()] = $user; } diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 7baed453439b..32e24c6fcb74 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -66,26 +66,21 @@ class User implements IUser { /** @var AccountMapper */ private $mapper; - /** @var AccountTermMapper */ - private $termMapper; - /** * User constructor. * * @param Account $account * @param AccountMapper $mapper - * @param AccountTermMapper $termMapper * @param null $emitter * @param IConfig|null $config * @param null $urlGenerator * @param EventDispatcher|null $eventDispatcher */ - public function __construct(Account $account, AccountMapper $mapper, AccountTermMapper $termMapper, $emitter = null, IConfig $config = null, + public function __construct(Account $account, AccountMapper $mapper, $emitter = null, IConfig $config = null, $urlGenerator = null, EventDispatcher $eventDispatcher = null ) { $this->account = $account; $this->mapper = $mapper; - $this->termMapper = $termMapper; $this->emitter = $emitter; $this->eventDispatcher = $eventDispatcher; if(is_null($config)) { @@ -449,7 +444,7 @@ public function triggerChange($feature, $value = null) { */ public function getSearchTerms() { $terms = []; - foreach ($this->termMapper->findByAccountId($this->account->getId()) as $term) { + foreach ($this->mapper->findByAccountId($this->account->getId()) as $term) { $terms[] = $term->getTerm(); } return $terms; @@ -460,6 +455,6 @@ public function getSearchTerms() { * @since 10.0.1 */ public function setSearchTerms(array $terms) { - $this->termMapper->setTermsForAccount($this->account->getId(), $terms); + $this->mapper->setTermsForAccount($this->account->getId(), $terms); } } diff --git a/lib/public/IGroupManager.php b/lib/public/IGroupManager.php index 61f444773969..7a563f578327 100644 --- a/lib/public/IGroupManager.php +++ b/lib/public/IGroupManager.php @@ -130,7 +130,7 @@ public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0 * @param string $search * @param int $limit * @param int $offset - * @return array an array of display names (value) and user ids (key) + * @return array an array of display names (value) and user objects * @since 10.0.1 */ public function findUsersInGroup($gid, $search = '', $limit = -1, $offset = 0); diff --git a/tests/lib/Files/Config/UserMountCacheTest.php b/tests/lib/Files/Config/UserMountCacheTest.php index da753cd669a1..5a3e37560ba9 100644 --- a/tests/lib/Files/Config/UserMountCacheTest.php +++ b/tests/lib/Files/Config/UserMountCacheTest.php @@ -14,12 +14,10 @@ use OC\Log; use OC\User\Account; use OC\User\AccountMapper; -use OC\User\AccountTermMapper; use OC\User\Manager; use OCP\Files\Config\ICachedMountInfo; use OCP\IConfig; use OCP\IDBConnection; -use OCP\ILogger; use OCP\IUserManager; use Test\TestCase; @@ -67,12 +65,9 @@ public function setUp() { ['u3', $a3], ]); - /** @var AccountTermMapper | \PHPUnit_Framework_MockObject_MockObject $accountTermMapper */ - $accountTermMapper = $this->createMock(AccountTermMapper::class); - /** @var Log $log */ $log = $this->createMock(Log::class); - $this->userManager = new Manager($config, $log, $accountMapper, $accountTermMapper); + $this->userManager = new Manager($config, $log, $accountMapper); $this->cache = new UserMountCache($this->connection, $this->userManager, $log); // hookup listener diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index b4d88c43d788..c49e6f501df4 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -22,6 +22,8 @@ */ namespace Test\Group; +use OC\Group\Database; +use OC\User\Manager; use OCP\IUser; use OCP\GroupInterface; @@ -94,7 +96,7 @@ public function testGet() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -107,7 +109,7 @@ public function testGetNoBackend() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $this->assertNull($manager->get('group1')); @@ -126,7 +128,7 @@ public function testGetNotExists() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -140,7 +142,7 @@ public function testGetDeleted() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -171,7 +173,7 @@ public function testGetMultipleBackends() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -202,7 +204,7 @@ public function testCreate() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -225,7 +227,7 @@ public function testCreateExists() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -250,7 +252,7 @@ public function testSearch() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -288,7 +290,7 @@ public function testSearchMultipleBackends() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -329,7 +331,7 @@ public function testSearchMultipleBackendsLimitAndOffset() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -346,7 +348,7 @@ public function testSearchResultExistsButGroupDoesNot() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->createMock(Database::class); $backend->expects($this->once()) ->method('getGroups') ->with('1') @@ -362,7 +364,7 @@ public function testSearchResultExistsButGroupDoesNot() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -402,7 +404,7 @@ public function testSearchBackendsForScope() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -439,8 +441,8 @@ public function testGetUserGroups() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -480,7 +482,7 @@ public function testGetUserGroupsWithDeletedGroup() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->createMock(Database::class); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -493,8 +495,8 @@ public function testGetUserGroupsWithDeletedGroup() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -526,8 +528,8 @@ public function testGetUserGroupsWithScope() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -556,8 +558,8 @@ public function testInGroup() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -580,8 +582,8 @@ public function testIsAdmin() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -604,8 +606,8 @@ public function testNotAdmin() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -640,8 +642,8 @@ public function testGetUserGroupsMultipleBackends() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -680,8 +682,8 @@ public function testDisplayNamesInGroupWithOneUserBackend() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('searchDisplayName') @@ -745,8 +747,8 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('searchDisplayName') @@ -812,8 +814,8 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('searchDisplayName') @@ -873,8 +875,8 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('get') @@ -917,8 +919,8 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('get') @@ -962,8 +964,8 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('get') @@ -1009,7 +1011,7 @@ public function testGetUserGroupsWithAddUser() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -1056,7 +1058,7 @@ public function testGetUserGroupsWithRemoveUser() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -1090,7 +1092,7 @@ public function testGetUserIdGroups() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -1120,7 +1122,7 @@ public function testGroupDisplayName() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager); $manager->addBackend($backend); @@ -1163,8 +1165,8 @@ public function testFindUsersInGroupWithOneUserBackend() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('find') @@ -1228,8 +1230,8 @@ public function testFindUsersInGroupWithOneUserBackendWithLimitSpecified() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('find') @@ -1295,8 +1297,8 @@ public function testFindUsersInGroupWithOneUserBackendWithLimitAndOffsetSpecifie /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('find') @@ -1356,8 +1358,8 @@ public function testFindUsersInGroupWithOneUserBackendAndSearchEmpty() { /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('get') @@ -1400,8 +1402,8 @@ public function testFindUsersInGroupWithOneUserBackendAndSearchEmptyAndLimitSpec /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('get') @@ -1445,8 +1447,8 @@ public function testFindUsersInGroupWithOneUserBackendAndSearchEmptyAndLimitAndO /** * @var \OC\User\Manager $userManager */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); + $userManager = $this->createMock(Manager::class); + $userBackend = $this->createMock(\OC_User_Backend::class); $userManager->expects($this->any()) ->method('get') diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index 643f7f6f8b0c..93d068af1073 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -12,8 +12,6 @@ use OC\Hooks\PublicEmitter; use OC\User\Account; use OC\User\AccountMapper; -use OC\User\AccountTerm; -use OC\User\AccountTermMapper; use OC\User\Backend; use OC\User\Database; use OC\User\User; @@ -34,8 +32,6 @@ class UserTest extends TestCase { /** @var AccountMapper | \PHPUnit_Framework_MockObject_MockObject */ private $accountMapper; - /** @var AccountTermMapper | \PHPUnit_Framework_MockObject_MockObject */ - private $accountTermMapper; /** @var Account */ private $account; /** @var User */ @@ -52,7 +48,6 @@ class UserTest extends TestCase { public function setUp() { parent::setUp(); $this->accountMapper = $this->createMock(AccountMapper::class); - $this->accountTermMapper = $this->createMock(AccountTermMapper::class); $this->config = $this->createMock(IConfig::class); $this->account = new Account(); $this->account->setUserId('foo'); @@ -63,7 +58,7 @@ public function setUp() { ->disableOriginalConstructor() ->getMock(); - $this->user = new User($this->account, $this->accountMapper, $this->accountTermMapper, $this->emitter, $this->config, $this->urlGenerator, $this->eventDispatcher); + $this->user = new User($this->account, $this->accountMapper, $this->emitter, $this->config, $this->urlGenerator, $this->eventDispatcher); } public function testDisplayName() { @@ -90,7 +85,7 @@ public function testSetPassword() { $account->expects($this->any())->method('__call')->with('getUserId')->willReturn('foo'); $backend->expects($this->once())->method('setPassword')->with('foo', 'bar')->willReturn(true); - $this->user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); + $this->user = new User($account, $this->accountMapper, null, $this->config); $this->assertTrue($this->user->setPassword('bar','')); $this->assertTrue($this->user->canChangePassword()); @@ -107,7 +102,7 @@ public function testSetPasswordNotSupported() { $account->expects($this->any())->method('__call')->with('getUserId')->willReturn('foo'); $backend->expects($this->once())->method('setPassword')->with('foo', 'bar')->willReturn(false); - $this->user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); + $this->user = new User($account, $this->accountMapper, null, $this->config); $this->assertFalse($this->user->setPassword('bar','')); $this->assertTrue($this->user->canChangePassword()); } @@ -149,7 +144,7 @@ public function testChangeAvatarSupported($expected, $implements, $canChange) { } })); - $user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); + $user = new User($account, $this->accountMapper, null, $this->config); $this->assertEquals($expected, $user->canChangeAvatar()); } @@ -201,7 +196,7 @@ public function testCanChangeDisplayName($expected, $implements) { } })); - $user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); + $user = new User($account, $this->accountMapper, null, $this->config); $this->assertEquals($expected, $user->canChangeDisplayName()); if ($expected) { @@ -237,7 +232,7 @@ public function testSetDisplayNameNotSupported() { return false; })); - $user = new User($account, $this->accountMapper, $this->accountTermMapper, null, $this->config); + $user = new User($account, $this->accountMapper, null, $this->config); $this->assertFalse($user->setDisplayName('Foo')); $this->assertEquals('foo',$user->getDisplayName()); } @@ -267,7 +262,7 @@ public function testSetPasswordHooks() { $account->expects($this->any())->method('__call')->with('getUserId')->willReturn('foo'); $backend->expects($this->once())->method('setPassword')->with('foo', 'bar')->willReturn(true); - $this->user = new User($account, $this->accountMapper, $this->accountTermMapper, $emitter, $this->config); + $this->user = new User($account, $this->accountMapper, $emitter, $this->config); $this->user->setPassword('bar',''); $this->assertEquals(2, $hooksCalled); From 08fd6c6c2dbce5f731f2f9c6c7e762cfdd413429 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Wed, 17 May 2017 10:54:21 +0100 Subject: [PATCH 18/30] Fix sync service command dependancies --- core/register_command.php | 2 +- lib/private/Server.php | 14 +++++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/core/register_command.php b/core/register_command.php index 80c4ba73c5ed..590560aec914 100644 --- a/core/register_command.php +++ b/core/register_command.php @@ -143,7 +143,7 @@ $application->add(new OC\Core\Command\User\Report(\OC::$server->getUserManager())); $application->add(new OC\Core\Command\User\ResetPassword(\OC::$server->getUserManager())); $application->add(new OC\Core\Command\User\Setting(\OC::$server->getUserManager(), \OC::$server->getConfig(), \OC::$server->getDatabaseConnection())); - $application->add(new OC\Core\Command\User\SyncBackend(new \OC\User\AccountMapper(\OC::$server->getDatabaseConnection()), \OC::$server->getConfig(), \OC::$server->getUserManager(), \OC::$server->getLogger())); + $application->add(new OC\Core\Command\User\SyncBackend(\OC::$server->getAccountMapper(), \OC::$server->getConfig(), \OC::$server->getUserManager(), \OC::$server->getLogger())); $application->add(new OC\Core\Command\Security\ListCertificates(\OC::$server->getCertificateManager(null), \OC::$server->getL10N('core'))); $application->add(new OC\Core\Command\Security\ImportCertificate(\OC::$server->getCertificateManager(null))); diff --git a/lib/private/Server.php b/lib/private/Server.php index 7e3d0e973a6f..83b2d73c875a 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -221,12 +221,13 @@ public function __construct($webRoot, \OC\Config $config) { return $c->getRootFolder(); }); }); + $this->registerService('AccountMapper', function(Server $c) { + return new AccountMapper($c->getDatabaseConnection(), new AccountTermMapper($c->getDatabaseConnection())); + }); $this->registerService('UserManager', function (Server $c) { $config = $c->getConfig(); $logger = $c->getLogger(); - $accountTermMapper = new AccountTermMapper($c->getDatabaseConnection()); - $accountMapper = new AccountMapper($c->getDatabaseConnection(), $accountTermMapper); - return new \OC\User\Manager($config, $logger, $accountMapper); + return new \OC\User\Manager($config, $logger, $c->getAccountMapper()); }); $this->registerService('GroupManager', function (Server $c) { $groupManager = new \OC\Group\Manager($this->getUserManager()); @@ -955,6 +956,13 @@ public function getUserManager() { return $this->query('UserManager'); } + /** + * @return \OC\User\AccountMapper + */ + public function getAccountMapper() { + return $this->query('AccountMapper'); + } + /** * @return \OC\Group\Manager */ From 2fb769835e89ee98d61e26f7db5b72479f29ebfb Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Wed, 17 May 2017 12:11:34 +0100 Subject: [PATCH 19/30] Fix sync service search term retrieval --- lib/private/User/Account.php | 24 -------------- lib/private/User/AccountMapper.php | 44 ++++++-------------------- lib/private/User/AccountTermMapper.php | 1 + lib/private/User/Manager.php | 13 ++++---- lib/private/User/SyncService.php | 5 +-- 5 files changed, 21 insertions(+), 66 deletions(-) diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index c2e138464ec8..14216d1378e3 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -43,7 +43,6 @@ * @method void setQuota(string $quota) * @method string getHome() * @method void setHome(string $home) - * @method string[] getSearchTerms() * * @package OC\User */ @@ -64,9 +63,6 @@ class Account extends Entity { protected $state; protected $home; - protected $terms = []; - private $_termsChanged = false; - public function __construct() { $this->addType('state', 'integer'); $this->addType('lastLogin', 'integer'); @@ -89,24 +85,4 @@ public function getBackendInstance() { return \OC::$server->getUserManager()->getBackend($backendClass); } - public function getUpdatedFields() { - $fields = parent::getUpdatedFields(); - unset($fields['terms']); - return $fields; - } - - public function haveTermsChanged() { - return $this->_termsChanged; - } - - /** - * @param string[] $terms - */ - public function setSearchTerms(array $terms) { - if(array_diff($terms, $this->terms)) { - $this->terms = $terms; - $this->_termsChanged = true; - } - } - } diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index ac24767df4fe..f6ec94082628 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -38,6 +38,16 @@ public function __construct(IDBConnection $db, AccountTermMapper $termMapper) { $this->termMapper = $termMapper; } + /** + * @param Account $entity + * @return Entity the deleted entity + */ + public function delete(Entity $entity) { + // First delete the search terms for this account + $this->termMapper->deleteTermsForAccount($entity->getId()); + return parent::delete($entity); + } + /** * Pass through call to term mapper to avoid needing to inject term mapper * @param $account_id @@ -56,40 +66,6 @@ public function findByAccountId($account_id) { return $this->termMapper->findByAccountId($account_id); } - /** - * @param Account $entity - * @return Entity the saved entity with the set id - */ - public function insert(Entity $entity) { - if($entity->haveTermsChanged()) { - $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); - } - // Then run the normal entity insert operation - return parent::insert($entity); - } - - /** - * @param Account $entity - * @return Entity the deleted entity - */ - public function delete(Entity $entity) { - // First delete the search terms for this account - $this->termMapper->deleteTermsForAccount($entity->getId()); - return parent::delete($entity); - } - - /** - * @param Account $entity - * @return Entity the saved entity with the set id - */ - public function update(Entity $entity) { - if($entity->haveTermsChanged()) { - $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); - } - // Then run the normal entity insert operation - return parent::update($entity); - } - /** * @param string $email * @return Account[] diff --git a/lib/private/User/AccountTermMapper.php b/lib/private/User/AccountTermMapper.php index f1e3ee6a8797..4e8ab6ac55f0 100644 --- a/lib/private/User/AccountTermMapper.php +++ b/lib/private/User/AccountTermMapper.php @@ -30,6 +30,7 @@ class AccountTermMapper extends Mapper { public function __construct(IDBConnection $db) { parent::__construct($db, 'account_terms', AccountTerm::class); } + /** * Sets search terms for a given account id, will always be lowercased * @param $account_id diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index a33dd33bd39e..dd555a89f20a 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -215,6 +215,7 @@ public function checkPassword($loginName, $password) { } catch(DoesNotExistException $ex) { $account = $this->newAccount($uid, $backend); } + // TODO always sync account with backend here to update displayname, email, search terms, home etc return $this->getUserObject($account); } } @@ -434,12 +435,6 @@ private function newAccount($uid, $backend) { $account->setQuota($quota); } } - if ($backend instanceof IProvidesExtendedSearchBackend) { - $terms = $backend->getSearchTerms($uid); - if (!empty($terms)) { - $account->setSearchTerms($terms); - } - } $home = false; if ($backend->implementsActions(Backend::GET_HOME)) { $home = $backend->getHome($uid); @@ -453,6 +448,12 @@ private function newAccount($uid, $backend) { } $account->setHome($home); $account = $this->accountMapper->insert($account); + if ($backend instanceof IProvidesExtendedSearchBackend) { + $terms = $backend->getSearchTerms($uid); + if (!empty($terms)) { + $this->accountMapper->setTermsForAccount($account->getId(), $terms); + } + } return $account; } diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index 604f0a5bff77..aca7fd63b6aa 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -110,8 +110,9 @@ public function run(\Closure $callback) { $this->mapper->update($a); } catch(DoesNotExistException $ex) { $a = $this->createNewAccount($uid); - $this->setupAccount($a, $uid); $this->mapper->insert($a); + $this->setupAccount($a, $uid); // TODO should be called before insert to reduce queries + $this->mapper->update($a); } // clean the user's preferences $this->cleanPreferences($uid); @@ -161,7 +162,7 @@ public function setupAccount(Account $a, $uid) { } // Check if backend supplies an additional search string if ($this->backend instanceof IProvidesExtendedSearchBackend) { - $a->setSearchTerms($this->backend->getSearchTerms($uid)); + $this->mapper->setTermsForAccount($a->getId(), $this->backend->getSearchTerms($uid)); } return $a; } From 20dd735acc40c0d0449dfc016d8b3e97e0c15552 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 17 May 2017 13:33:19 +0200 Subject: [PATCH 20/30] Revert "Fix sync service search term retrieval" This reverts commit 2fb769835e89ee98d61e26f7db5b72479f29ebfb. --- lib/private/User/Account.php | 24 ++++++++++++++ lib/private/User/AccountMapper.php | 44 ++++++++++++++++++++------ lib/private/User/AccountTermMapper.php | 1 - lib/private/User/Manager.php | 13 ++++---- lib/private/User/SyncService.php | 5 ++- 5 files changed, 66 insertions(+), 21 deletions(-) diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index 14216d1378e3..c2e138464ec8 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -43,6 +43,7 @@ * @method void setQuota(string $quota) * @method string getHome() * @method void setHome(string $home) + * @method string[] getSearchTerms() * * @package OC\User */ @@ -63,6 +64,9 @@ class Account extends Entity { protected $state; protected $home; + protected $terms = []; + private $_termsChanged = false; + public function __construct() { $this->addType('state', 'integer'); $this->addType('lastLogin', 'integer'); @@ -85,4 +89,24 @@ public function getBackendInstance() { return \OC::$server->getUserManager()->getBackend($backendClass); } + public function getUpdatedFields() { + $fields = parent::getUpdatedFields(); + unset($fields['terms']); + return $fields; + } + + public function haveTermsChanged() { + return $this->_termsChanged; + } + + /** + * @param string[] $terms + */ + public function setSearchTerms(array $terms) { + if(array_diff($terms, $this->terms)) { + $this->terms = $terms; + $this->_termsChanged = true; + } + } + } diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index f6ec94082628..ac24767df4fe 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -38,16 +38,6 @@ public function __construct(IDBConnection $db, AccountTermMapper $termMapper) { $this->termMapper = $termMapper; } - /** - * @param Account $entity - * @return Entity the deleted entity - */ - public function delete(Entity $entity) { - // First delete the search terms for this account - $this->termMapper->deleteTermsForAccount($entity->getId()); - return parent::delete($entity); - } - /** * Pass through call to term mapper to avoid needing to inject term mapper * @param $account_id @@ -66,6 +56,40 @@ public function findByAccountId($account_id) { return $this->termMapper->findByAccountId($account_id); } + /** + * @param Account $entity + * @return Entity the saved entity with the set id + */ + public function insert(Entity $entity) { + if($entity->haveTermsChanged()) { + $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); + } + // Then run the normal entity insert operation + return parent::insert($entity); + } + + /** + * @param Account $entity + * @return Entity the deleted entity + */ + public function delete(Entity $entity) { + // First delete the search terms for this account + $this->termMapper->deleteTermsForAccount($entity->getId()); + return parent::delete($entity); + } + + /** + * @param Account $entity + * @return Entity the saved entity with the set id + */ + public function update(Entity $entity) { + if($entity->haveTermsChanged()) { + $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); + } + // Then run the normal entity insert operation + return parent::update($entity); + } + /** * @param string $email * @return Account[] diff --git a/lib/private/User/AccountTermMapper.php b/lib/private/User/AccountTermMapper.php index 4e8ab6ac55f0..f1e3ee6a8797 100644 --- a/lib/private/User/AccountTermMapper.php +++ b/lib/private/User/AccountTermMapper.php @@ -30,7 +30,6 @@ class AccountTermMapper extends Mapper { public function __construct(IDBConnection $db) { parent::__construct($db, 'account_terms', AccountTerm::class); } - /** * Sets search terms for a given account id, will always be lowercased * @param $account_id diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index dd555a89f20a..a33dd33bd39e 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -215,7 +215,6 @@ public function checkPassword($loginName, $password) { } catch(DoesNotExistException $ex) { $account = $this->newAccount($uid, $backend); } - // TODO always sync account with backend here to update displayname, email, search terms, home etc return $this->getUserObject($account); } } @@ -435,6 +434,12 @@ private function newAccount($uid, $backend) { $account->setQuota($quota); } } + if ($backend instanceof IProvidesExtendedSearchBackend) { + $terms = $backend->getSearchTerms($uid); + if (!empty($terms)) { + $account->setSearchTerms($terms); + } + } $home = false; if ($backend->implementsActions(Backend::GET_HOME)) { $home = $backend->getHome($uid); @@ -448,12 +453,6 @@ private function newAccount($uid, $backend) { } $account->setHome($home); $account = $this->accountMapper->insert($account); - if ($backend instanceof IProvidesExtendedSearchBackend) { - $terms = $backend->getSearchTerms($uid); - if (!empty($terms)) { - $this->accountMapper->setTermsForAccount($account->getId(), $terms); - } - } return $account; } diff --git a/lib/private/User/SyncService.php b/lib/private/User/SyncService.php index aca7fd63b6aa..604f0a5bff77 100644 --- a/lib/private/User/SyncService.php +++ b/lib/private/User/SyncService.php @@ -110,9 +110,8 @@ public function run(\Closure $callback) { $this->mapper->update($a); } catch(DoesNotExistException $ex) { $a = $this->createNewAccount($uid); + $this->setupAccount($a, $uid); $this->mapper->insert($a); - $this->setupAccount($a, $uid); // TODO should be called before insert to reduce queries - $this->mapper->update($a); } // clean the user's preferences $this->cleanPreferences($uid); @@ -162,7 +161,7 @@ public function setupAccount(Account $a, $uid) { } // Check if backend supplies an additional search string if ($this->backend instanceof IProvidesExtendedSearchBackend) { - $this->mapper->setTermsForAccount($a->getId(), $this->backend->getSearchTerms($uid)); + $a->setSearchTerms($this->backend->getSearchTerms($uid)); } return $a; } From 2aab10dbd2e430fdf378b8eadf05f5135679e068 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 17 May 2017 13:42:50 +0200 Subject: [PATCH 21/30] use id from account insert to set search terms correctly --- lib/private/User/AccountMapper.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index ac24767df4fe..54e2655b2597 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -61,11 +61,14 @@ public function findByAccountId($account_id) { * @return Entity the saved entity with the set id */ public function insert(Entity $entity) { - if($entity->haveTermsChanged()) { + // run the normal entity insert operation to get an id + $entity = parent::insert($entity); + + /** @var Account $entity */ + if ($entity->haveTermsChanged()) { $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); } - // Then run the normal entity insert operation - return parent::insert($entity); + return $entity; } /** @@ -80,10 +83,10 @@ public function delete(Entity $entity) { /** * @param Account $entity - * @return Entity the saved entity with the set id + * @return Entity the updated entity */ public function update(Entity $entity) { - if($entity->haveTermsChanged()) { + if ($entity->haveTermsChanged()) { $this->termMapper->setTermsForAccount($entity->getId(), $entity->getSearchTerms()); } // Then run the normal entity insert operation From 7a07d705434d1322f703398c950c78cee14d97be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 17 May 2017 13:58:50 +0200 Subject: [PATCH 22/30] update comments --- .../lib/Controller/ShareesController.php | 2 +- apps/files_sharing/tests/API/ShareesTest.php | 2 +- core/Migrations/Version20170516100103.php | 21 +++++++++++++++++++ lib/private/User/AccountMapper.php | 3 ++- lib/private/User/AccountTerm.php | 3 ++- lib/private/User/AccountTermMapper.php | 2 ++ lib/private/User/Manager.php | 2 ++ lib/private/User/User.php | 1 + 8 files changed, 32 insertions(+), 4 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareesController.php b/apps/files_sharing/lib/Controller/ShareesController.php index 15d5da923cba..7c7fd122dae9 100644 --- a/apps/files_sharing/lib/Controller/ShareesController.php +++ b/apps/files_sharing/lib/Controller/ShareesController.php @@ -3,8 +3,8 @@ * @author Björn Schießle * @author Joas Schilling * @author Roeland Jago Douma - * @author Roeland Jago Douma * @author Thomas Müller + * @author Tom Needham * @author Vincent Petry * * @copyright Copyright (c) 2017, ownCloud GmbH diff --git a/apps/files_sharing/tests/API/ShareesTest.php b/apps/files_sharing/tests/API/ShareesTest.php index 4a1c70bd1867..673f69d25016 100644 --- a/apps/files_sharing/tests/API/ShareesTest.php +++ b/apps/files_sharing/tests/API/ShareesTest.php @@ -3,8 +3,8 @@ * @author Björn Schießle * @author Joas Schilling * @author Roeland Jago Douma - * @author Roeland Jago Douma * @author Thomas Müller + * @author Tom Needham * @author Vincent Petry * * @copyright Copyright (c) 2017, ownCloud GmbH diff --git a/core/Migrations/Version20170516100103.php b/core/Migrations/Version20170516100103.php index d08f7cd7f374..ee89c6fa26b2 100644 --- a/core/Migrations/Version20170516100103.php +++ b/core/Migrations/Version20170516100103.php @@ -1,4 +1,25 @@ + * @author Tom Needham + * + * @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 + * + */ + namespace OC\Migrations; use Doctrine\DBAL\Schema\Schema; diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 54e2655b2597..dda1fa8445a6 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -1,6 +1,8 @@ * @author Thomas Müller + * @author Tom Needham * * @copyright Copyright (c) 2017, ownCloud GmbH * @license AGPL-3.0 @@ -19,7 +21,6 @@ * */ - namespace OC\User; diff --git a/lib/private/User/AccountTerm.php b/lib/private/User/AccountTerm.php index d13a8d556fa3..9972fbda3291 100644 --- a/lib/private/User/AccountTerm.php +++ b/lib/private/User/AccountTerm.php @@ -1,6 +1,7 @@ + * @author Tom Needham * * @copyright Copyright (c) 2017, ownCloud GmbH * @license AGPL-3.0 @@ -25,7 +26,7 @@ use OCP\AppFramework\Db\Entity; /** - * Class Account + * Class AccountTerm * * @method int getAccountId() * @method void setAccountId(int $accountId) diff --git a/lib/private/User/AccountTermMapper.php b/lib/private/User/AccountTermMapper.php index f1e3ee6a8797..8ba4c4b70279 100644 --- a/lib/private/User/AccountTermMapper.php +++ b/lib/private/User/AccountTermMapper.php @@ -1,6 +1,7 @@ + * @author Tom Needham * * @copyright Copyright (c) 2017, ownCloud GmbH * @license AGPL-3.0 @@ -30,6 +31,7 @@ class AccountTermMapper extends Mapper { public function __construct(IDBConnection $db) { parent::__construct($db, 'account_terms', AccountTerm::class); } + /** * Sets search terms for a given account id, will always be lowercased * @param $account_id diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index a33dd33bd39e..de8c54590379 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -8,6 +8,7 @@ * @author Morris Jobke * @author Robin Appelman * @author Thomas Müller + * @author Tom Needham * @author Victor Dubiniuk * @author Vincent Chan * @author Vincent Petry @@ -215,6 +216,7 @@ public function checkPassword($loginName, $password) { } catch(DoesNotExistException $ex) { $account = $this->newAccount($uid, $backend); } + // TODO always sync account with backend here to update displayname, email, search terms, home etc. user_ldap currently updates user metadata on login, core should take care of updating accounts on a successful login return $this->getUserObject($account); } } diff --git a/lib/private/User/User.php b/lib/private/User/User.php index 32e24c6fcb74..403aab4a5a23 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -8,6 +8,7 @@ * @author Morris Jobke * @author Robin Appelman * @author Thomas Müller + * @author Tom Needham * @author Victor Dubiniuk * @author Vincent Petry * From dadcd78ba265953588b8f9c80c8747651f152cf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 17 May 2017 14:03:55 +0200 Subject: [PATCH 23/30] minor doc changes --- lib/private/Group/Manager.php | 2 +- lib/private/User/AccountMapper.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 48557d2a72f8..4b90edb58ce2 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -355,7 +355,7 @@ public function getUserGroupIds($user, $scope = null) { * @param string $search * @param int $limit * @param int $offset - * @return array of user objects + * @return \OC\User\User[] */ public function findUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) { $group = $this->get($gid); diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index dda1fa8445a6..79029fc28cf6 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -40,7 +40,7 @@ public function __construct(IDBConnection $db, AccountTermMapper $termMapper) { } /** - * Pass through call to term mapper to avoid needing to inject term mapper + * Delegate to term mapper to avoid needing to inject term mapper * @param $account_id * @param array $terms */ @@ -49,7 +49,7 @@ public function setTermsForAccount($account_id, array $terms) { } /** - * Pass through call to term mapper to avoid needing to inject term mapper + * Delegate to term mapper to avoid needing to inject term mapper * @param $account_id * @return AccountTerm[] $terms */ From 869eacbba5bcd1da20318bb3dfca742843c398f6 Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Wed, 17 May 2017 13:46:24 +0100 Subject: [PATCH 24/30] Add missing getting for account search terms --- lib/private/User/Account.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/private/User/Account.php b/lib/private/User/Account.php index c2e138464ec8..013233f8f58f 100644 --- a/lib/private/User/Account.php +++ b/lib/private/User/Account.php @@ -43,7 +43,6 @@ * @method void setQuota(string $quota) * @method string getHome() * @method void setHome(string $home) - * @method string[] getSearchTerms() * * @package OC\User */ @@ -64,7 +63,7 @@ class Account extends Entity { protected $state; protected $home; - protected $terms = []; + private $terms = []; private $_termsChanged = false; public function __construct() { @@ -109,4 +108,11 @@ public function setSearchTerms(array $terms) { } } + /** + * @return string[] + */ + public function getSearchTerms() { + return $this->terms; + } + } From 14cc9adb8e572372039315d59112c78ea1d40d0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 17 May 2017 14:55:55 +0200 Subject: [PATCH 25/30] codestyle --- lib/private/User/AccountTermMapper.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/private/User/AccountTermMapper.php b/lib/private/User/AccountTermMapper.php index 8ba4c4b70279..b6158dc46c0c 100644 --- a/lib/private/User/AccountTermMapper.php +++ b/lib/private/User/AccountTermMapper.php @@ -55,8 +55,7 @@ public function setTermsForAccount($account_id, array $terms) { */ public function deleteTermsForAccount($account_id) { $qb = $this->db->getQueryBuilder(); - $qb - ->delete($this->getTableName()) + $qb->delete($this->getTableName()) ->where($qb->expr()->eq('account_id', $qb->createNamedParameter($account_id))) ->execute(); } From 8865e33ee36b0ce43a1f25f5bfee9c3b6c0981ef Mon Sep 17 00:00:00 2001 From: Tom Needham Date: Wed, 17 May 2017 17:45:29 +0100 Subject: [PATCH 26/30] Catch exact sharee matches for email address searches --- apps/files_sharing/lib/Controller/ShareesController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/files_sharing/lib/Controller/ShareesController.php b/apps/files_sharing/lib/Controller/ShareesController.php index 7c7fd122dae9..7c585e9ce05f 100644 --- a/apps/files_sharing/lib/Controller/ShareesController.php +++ b/apps/files_sharing/lib/Controller/ShareesController.php @@ -165,7 +165,8 @@ protected function getUsers($search) { $foundUserById = false; $lowerSearch = strtolower($search); foreach ($users as $uid => $user) { - if (strtolower($uid) === $lowerSearch || strtolower($user->getDisplayName()) === $lowerSearch || strtolower($user->getEmailAddress())) { + /* @var $user IUser */ + if (strtolower($uid) === $lowerSearch || strtolower($user->getDisplayName()) === $lowerSearch || strtolower($user->getEMailAddress()) === $lowerSearch) { if (strtolower($uid) === $lowerSearch) { $foundUserById = true; } From 06d7c36c66d18a888f318c7f8dcd55f77749c54c Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 18 May 2017 10:24:07 +0200 Subject: [PATCH 27/30] Use left join for account search Because searching by email do not require terms to exist. --- lib/private/User/AccountMapper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 79029fc28cf6..976867b10359 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -152,7 +152,7 @@ public function find($pattern, $limit, $offset) { $qb->select(['user_id', 'lower_user_id', 'display_name', 'email', 'last_login', 'backend', 'state', 'quota', 'home']) ->selectAlias('a.id', 'id') ->from($this->getTableName(), 'a') - ->join('a', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) + ->leftJoin('a', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) ->where($qb->expr()->Like('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) From f4b1dc146366595bb291411804d20000c6d7e9ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 18 May 2017 13:11:03 +0200 Subject: [PATCH 28/30] add indexes, query correct column --- core/Migrations/Version20170516100103.php | 13 +++++++++++++ lib/private/User/AccountMapper.php | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/core/Migrations/Version20170516100103.php b/core/Migrations/Version20170516100103.php index ee89c6fa26b2..787c76465a4f 100644 --- a/core/Migrations/Version20170516100103.php +++ b/core/Migrations/Version20170516100103.php @@ -58,5 +58,18 @@ public function changeSchema(Schema $schema, array $options) { $table->addIndex(['term'], 'term_index'); } + + if ($schema->hasTable("${prefix}accounts")) { + $table = $schema->getTable("${prefix}accounts"); + if (!$table->hasIndex('lower_user_id_index')) { + $table->addUniqueIndex(['lower_user_id'], 'lower_user_id_index'); + } + if (!$table->hasIndex('display_name_index')) { + $table->addIndex(['display_name'], 'display_name_index'); + } + if (!$table->hasIndex('email_index')) { + $table->addIndex(['email'], 'email_index'); + } + } } } diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 976867b10359..66f28db075a8 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -153,7 +153,7 @@ public function find($pattern, $limit, $offset) { ->selectAlias('a.id', 'id') ->from($this->getTableName(), 'a') ->leftJoin('a', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) - ->where($qb->expr()->Like('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) + ->where($qb->expr()->Like('lower_user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) From 148d443f83ec136cfe786d3fac76368519073460 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 18 May 2017 14:23:26 +0200 Subject: [PATCH 29/30] add index, introduce medial search option, add sample config --- config/config.sample.php | 7 ++++++ core/Migrations/Version20170221114437.php | 3 ++- lib/private/Server.php | 2 +- lib/private/User/AccountMapper.php | 29 +++++++++++++++++------ tests/lib/Traits/UserTrait.php | 4 +++- 5 files changed, 35 insertions(+), 10 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index 5f82fee0e407..00f307e46d4b 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -256,6 +256,13 @@ */ 'lost_password_link' => 'https://example.org/link/to/password/reset', +/** + * Allow medial search on account properties like display name, user id, email, + * and other search terms. Allows finding 'Alice' when searching for 'lic'. + * May slow down user search. + */ +'accounts.enable_medial_search' => false, + /** * Mail Parameters * diff --git a/core/Migrations/Version20170221114437.php b/core/Migrations/Version20170221114437.php index ce4183628d67..1ceebec287de 100644 --- a/core/Migrations/Version20170221114437.php +++ b/core/Migrations/Version20170221114437.php @@ -17,9 +17,10 @@ class Version20170221114437 implements ISimpleMigration { */ public function run(IOutput $out) { $backend = new Database(); - $accountMapper = new AccountMapper(\OC::$server->getDatabaseConnection(), new AccountTermMapper(\OC::$server->getDatabaseConnection())); $config = \OC::$server->getConfig(); $logger = \OC::$server->getLogger(); + $connection = \OC::$server->getDatabaseConnection(); + $accountMapper = new AccountMapper($config, $connection, new AccountTermMapper($connection)); $syncService = new SyncService($accountMapper, $backend, $config, $logger); // insert/update known users diff --git a/lib/private/Server.php b/lib/private/Server.php index 83b2d73c875a..aa30b6862800 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -222,7 +222,7 @@ public function __construct($webRoot, \OC\Config $config) { }); }); $this->registerService('AccountMapper', function(Server $c) { - return new AccountMapper($c->getDatabaseConnection(), new AccountTermMapper($c->getDatabaseConnection())); + return new AccountMapper($c->getConfig(), $c->getDatabaseConnection(), new AccountTermMapper($c->getDatabaseConnection())); }); $this->registerService('UserManager', function (Server $c) { $config = $c->getConfig(); diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index 66f28db075a8..d710199cdfa8 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -27,15 +27,20 @@ use OC\DB\QueryBuilder\Literal; use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\Mapper; +use OCP\IConfig; use OCP\IDBConnection; class AccountMapper extends Mapper { + /* @var IConfig */ + protected $config; + /* @var AccountTermMapper */ protected $termMapper; - public function __construct(IDBConnection $db, AccountTermMapper $termMapper) { + public function __construct(IConfig $config, IDBConnection $db, AccountTermMapper $termMapper) { parent::__construct($db, 'accounts', Account::class); + $this->config = $config; $this->termMapper = $termMapper; } @@ -134,6 +139,7 @@ public function search($fieldName, $pattern, $limit, $offset) { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) + // TODO use medial search config option here as well ->where($qb->expr()->iLike($fieldName, $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orderBy($fieldName); @@ -148,17 +154,26 @@ public function search($fieldName, $pattern, $limit, $offset) { */ public function find($pattern, $limit, $offset) { $qb = $this->db->getQueryBuilder(); - $lowerPattern = strtolower($pattern); $qb->select(['user_id', 'lower_user_id', 'display_name', 'email', 'last_login', 'backend', 'state', 'quota', 'home']) ->selectAlias('a.id', 'id') ->from($this->getTableName(), 'a') ->leftJoin('a', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) - ->where($qb->expr()->Like('lower_user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) - ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) ->orderBy('display_name'); + $lowerPattern = strtolower($pattern); + $allowMedialSearches = $this->config->getSystemValue('accounts.enable_medial_search', false); + if ($allowMedialSearches) { + $qb->where($qb->expr()->like('lower_user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) + ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))); + } else { + $qb->where($qb->expr()->like('lower_user_id', $qb->createNamedParameter($this->db->escapeLikeParameter($lowerPattern) . '%'))) + ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter($this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter($this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter($this->db->escapeLikeParameter($lowerPattern) . '%'))); + } + return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset); } @@ -205,7 +220,7 @@ public function callForAllUsers($callback, $search, $onlySeen) { $qb->select(['*']) ->from($this->getTableName()); - if ($search) { + if ($search) { // TODO use medial search config option here as well $qb->where($qb->expr()->iLike('user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($search) . '%'))); } diff --git a/tests/lib/Traits/UserTrait.php b/tests/lib/Traits/UserTrait.php index 171de3a32c7c..a9612c37d2a4 100644 --- a/tests/lib/Traits/UserTrait.php +++ b/tests/lib/Traits/UserTrait.php @@ -10,6 +10,7 @@ use OC\User\AccountTermMapper; use OC\User\User; +use OCP\IConfig; use Test\Util\User\Dummy; use Test\Util\User\MemoryAccountMapper; @@ -39,7 +40,8 @@ protected function createUser($name, $password = null) { protected function setUpUserTrait() { $db = \OC::$server->getDatabaseConnection(); - $accountMapper = new MemoryAccountMapper($db, new AccountTermMapper($db)); + $config = $this->createMock(IConfig::class); + $accountMapper = new MemoryAccountMapper($config, $db, new AccountTermMapper($db)); $accountMapper->testCaseName = get_class($this); $this->previousUserManagerInternals = \OC::$server->getUserManager() ->reset($accountMapper, [Dummy::class => new Dummy()]); From d9fc93c694fa46bc7cbb1cf090271394a99f9897 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Thu, 18 May 2017 17:19:02 +0200 Subject: [PATCH 30/30] remove optional medial search, update tech debt todos --- core/Migrations/Version20170221114437.php | 3 +-- lib/private/Server.php | 2 +- lib/private/User/AccountMapper.php | 31 ++++++++--------------- tests/lib/Traits/UserTrait.php | 4 +-- 4 files changed, 13 insertions(+), 27 deletions(-) diff --git a/core/Migrations/Version20170221114437.php b/core/Migrations/Version20170221114437.php index 1ceebec287de..036993e9ed18 100644 --- a/core/Migrations/Version20170221114437.php +++ b/core/Migrations/Version20170221114437.php @@ -6,7 +6,6 @@ use OC\User\AccountTermMapper; use OC\User\Database; use OC\User\SyncService; -use OCP\IConfig; use OCP\Migration\ISimpleMigration; use OCP\Migration\IOutput; @@ -20,7 +19,7 @@ public function run(IOutput $out) { $config = \OC::$server->getConfig(); $logger = \OC::$server->getLogger(); $connection = \OC::$server->getDatabaseConnection(); - $accountMapper = new AccountMapper($config, $connection, new AccountTermMapper($connection)); + $accountMapper = new AccountMapper($connection, new AccountTermMapper($connection)); $syncService = new SyncService($accountMapper, $backend, $config, $logger); // insert/update known users diff --git a/lib/private/Server.php b/lib/private/Server.php index aa30b6862800..83b2d73c875a 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -222,7 +222,7 @@ public function __construct($webRoot, \OC\Config $config) { }); }); $this->registerService('AccountMapper', function(Server $c) { - return new AccountMapper($c->getConfig(), $c->getDatabaseConnection(), new AccountTermMapper($c->getDatabaseConnection())); + return new AccountMapper($c->getDatabaseConnection(), new AccountTermMapper($c->getDatabaseConnection())); }); $this->registerService('UserManager', function (Server $c) { $config = $c->getConfig(); diff --git a/lib/private/User/AccountMapper.php b/lib/private/User/AccountMapper.php index d710199cdfa8..d16a5af255e3 100644 --- a/lib/private/User/AccountMapper.php +++ b/lib/private/User/AccountMapper.php @@ -32,15 +32,11 @@ class AccountMapper extends Mapper { - /* @var IConfig */ - protected $config; - /* @var AccountTermMapper */ protected $termMapper; - public function __construct(IConfig $config, IDBConnection $db, AccountTermMapper $termMapper) { + public function __construct(IDBConnection $db, AccountTermMapper $termMapper) { parent::__construct($db, 'accounts', Account::class); - $this->config = $config; $this->termMapper = $termMapper; } @@ -139,7 +135,7 @@ public function search($fieldName, $pattern, $limit, $offset) { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) - // TODO use medial search config option here as well + // TODO check performance on large installs because like with starting % cannot use indexes ->where($qb->expr()->iLike($fieldName, $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) ->orderBy($fieldName); @@ -153,26 +149,18 @@ public function search($fieldName, $pattern, $limit, $offset) { * @return Account[] */ public function find($pattern, $limit, $offset) { + $lowerPattern = strtolower($pattern); $qb = $this->db->getQueryBuilder(); $qb->select(['user_id', 'lower_user_id', 'display_name', 'email', 'last_login', 'backend', 'state', 'quota', 'home']) ->selectAlias('a.id', 'id') ->from($this->getTableName(), 'a') ->leftJoin('a', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id')) - ->orderBy('display_name'); + ->orderBy('display_name') + ->where($qb->expr()->like('lower_user_id', $qb->createNamedParameter($this->db->escapeLikeParameter($lowerPattern) . '%'))) + ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter($this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter($this->db->escapeLikeParameter($pattern) . '%'))) + ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter($this->db->escapeLikeParameter($lowerPattern) . '%'))); - $lowerPattern = strtolower($pattern); - $allowMedialSearches = $this->config->getSystemValue('accounts.enable_medial_search', false); - if ($allowMedialSearches) { - $qb->where($qb->expr()->like('lower_user_id', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))) - ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($lowerPattern) . '%'))); - } else { - $qb->where($qb->expr()->like('lower_user_id', $qb->createNamedParameter($this->db->escapeLikeParameter($lowerPattern) . '%'))) - ->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter($this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter($this->db->escapeLikeParameter($pattern) . '%'))) - ->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter($this->db->escapeLikeParameter($lowerPattern) . '%'))); - } return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset); } @@ -220,8 +208,9 @@ public function callForAllUsers($callback, $search, $onlySeen) { $qb->select(['*']) ->from($this->getTableName()); - if ($search) { // TODO use medial search config option here as well + if ($search) { $qb->where($qb->expr()->iLike('user_id', + // TODO check performance on large installs because like with starting % cannot use indexes $qb->createNamedParameter('%' . $this->db->escapeLikeParameter($search) . '%'))); } if ($onlySeen) { diff --git a/tests/lib/Traits/UserTrait.php b/tests/lib/Traits/UserTrait.php index a9612c37d2a4..171de3a32c7c 100644 --- a/tests/lib/Traits/UserTrait.php +++ b/tests/lib/Traits/UserTrait.php @@ -10,7 +10,6 @@ use OC\User\AccountTermMapper; use OC\User\User; -use OCP\IConfig; use Test\Util\User\Dummy; use Test\Util\User\MemoryAccountMapper; @@ -40,8 +39,7 @@ protected function createUser($name, $password = null) { protected function setUpUserTrait() { $db = \OC::$server->getDatabaseConnection(); - $config = $this->createMock(IConfig::class); - $accountMapper = new MemoryAccountMapper($config, $db, new AccountTermMapper($db)); + $accountMapper = new MemoryAccountMapper($db, new AccountTermMapper($db)); $accountMapper->testCaseName = get_class($this); $this->previousUserManagerInternals = \OC::$server->getUserManager() ->reset($accountMapper, [Dummy::class => new Dummy()]);