Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

User backend extendable account searching #27906

Merged
merged 30 commits into from
May 18, 2017
Merged

User backend extendable account searching #27906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
e3c3261
Add find methods to user manager and group manager
tomneedham May 8, 2017
53bef4a
Add indexes for improved search performance and use lower uid in query
tomneedham May 11, 2017
9775519
Add search attribute to user interface
tomneedham May 11, 2017
ff08373
Add search attribute to user interface
tomneedham May 11, 2017
d5933a3
Update sharee unit tests for custom search attribute support
tomneedham May 12, 2017
53fc71e
Renamed search attribute backend interface
tomneedham May 12, 2017
a73ef47
Add tests for group|user manager find methods
tomneedham May 12, 2017
7a88187
Update account after changing search attributes string
tomneedham May 12, 2017
387fd9e
WIP account_terms table
tomneedham May 16, 2017
360753a
minor fixes
butonic May 16, 2017
8ca31fe
use dedicated AccountTerm Mapper and Entity, use terms during search
butonic May 16, 2017
3332789
fix di
butonic May 16, 2017
a73f2f1
fix fix for di
butonic May 16, 2017
c916482
return entity en insert, update and delete
butonic May 16, 2017
8dbeb54
fix insert
butonic May 16, 2017
2f1468c
Inject account term mapper into account mapper and update tests
tomneedham May 16, 2017
dd377c9
Update tests and clean code
tomneedham May 17, 2017
08fd6c6
Fix sync service command dependancies
tomneedham May 17, 2017
2fb7698
Fix sync service search term retrieval
tomneedham May 17, 2017
20dd735
Revert "Fix sync service search term retrieval"
butonic May 17, 2017
2aab10d
use id from account insert to set search terms correctly
butonic May 17, 2017
7a07d70
update comments
butonic May 17, 2017
dadcd78
minor doc changes
butonic May 17, 2017
869eacb
Add missing getting for account search terms
tomneedham May 17, 2017
14cc9ad
codestyle
butonic May 17, 2017
8865e33
Catch exact sharee matches for email address searches
tomneedham May 17, 2017
06d7c36
Use left join for account search
May 18, 2017
f4b1dc1
add indexes, query correct column
butonic May 18, 2017
148d443
add index, introduce medial search option, add sample config
butonic May 18, 2017
d9fc93c
remove optional medial search, update tech debt todos
butonic May 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions config/config.sample.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down
3 changes: 2 additions & 1 deletion core/Migrations/Version20170221114437.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
29 changes: 22 additions & 7 deletions lib/private/User/AccountMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);

Expand All @@ -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);
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

something for much later ?

Copy link
Member

Choose a reason for hiding this comment

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

well if the medial search was only experimental we can get rid of all the TODOs as well as the config option stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, yeah... if the search is running entirely within OC tables then the option is not useful any more.

It was added for people with special requirements but not default due to perf impact.

$qb->where($qb->expr()->iLike('user_id',
$qb->createNamedParameter('%' . $this->db->escapeLikeParameter($search) . '%')));
}
Expand Down
4 changes: 3 additions & 1 deletion tests/lib/Traits/UserTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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()]);
Expand Down