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.

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

Stable10 reenable medial search #28064

Merged
merged 2 commits into from
Jun 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,10 @@
/**
* 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.
* May slow down user search. Disable this if you encounter slow username search
* in the sharing dialog.
*/
'accounts.enable_medial_search' => false,
'accounts.enable_medial_search' => true,

/**
* Mail Parameters
Expand Down
3 changes: 2 additions & 1 deletion core/Migrations/Version20170221114437.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use OC\User\AccountTermMapper;
use OC\User\Database;
use OC\User\SyncService;
use OCP\IConfig;
use OCP\Migration\ISimpleMigration;
use OCP\Migration\IOutput;

Expand All @@ -19,7 +20,7 @@ public function run(IOutput $out) {
$config = \OC::$server->getConfig();
$logger = \OC::$server->getLogger();
$connection = \OC::$server->getDatabaseConnection();
$accountMapper = new AccountMapper($connection, new AccountTermMapper($connection));
$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
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
27 changes: 19 additions & 8 deletions lib/private/User/AccountMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,15 @@

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 @@ -135,7 +139,6 @@ public function search($fieldName, $pattern, $limit, $offset) {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->getTableName())
// 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);

Expand All @@ -149,17 +152,26 @@ public function search($fieldName, $pattern, $limit, $offset) {
* @return Account[]
*/
public function find($pattern, $limit = null, $offset = null) {
$lowerPattern = strtolower($pattern);

$allowMedialSearches = $this->config->getSystemValue('accounts.enable_medial_search', true);
if ($allowMedialSearches) {
$parameter = '%' . $this->db->escapeLikeParameter($pattern) . '%';
$loweredParameter = '%' . $this->db->escapeLikeParameter(strtolower($pattern)) . '%';
} else {
$parameter = $this->db->escapeLikeParameter($pattern) . '%';
$loweredParameter = $this->db->escapeLikeParameter(strtolower($pattern)) . '%';
}

$qb = $this->db->getQueryBuilder();
$qb->selectAlias('DISTINCT a.id', 'id')
->addSelect(['user_id', 'lower_user_id', 'display_name', 'email', 'last_login', 'backend', 'state', 'quota', 'home'])
->from($this->getTableName(), 'a')
->leftJoin('a', 'account_terms', 't', $qb->expr()->eq('a.id', 't.account_id'))
->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) . '%')));
->where($qb->expr()->like('lower_user_id', $qb->createNamedParameter($loweredParameter)))
->orWhere($qb->expr()->iLike('display_name', $qb->createNamedParameter($parameter)))
->orWhere($qb->expr()->iLike('email', $qb->createNamedParameter($parameter)))
->orWhere($qb->expr()->like('t.term', $qb->createNamedParameter($loweredParameter)));

return $this->findEntities($qb->getSQL(), $qb->getParameters(), $limit, $offset);
}
Expand Down Expand Up @@ -209,7 +221,6 @@ public function callForAllUsers($callback, $search, $onlySeen) {

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) {
Expand Down
4 changes: 3 additions & 1 deletion tests/lib/Traits/UserTrait.php
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
29 changes: 16 additions & 13 deletions tests/lib/User/AccountMapperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use OC\User\Account;
use OC\User\AccountMapper;
use OC\User\AccountTermMapper;
use OCP\IConfig;
use OCP\IDBConnection;
use Test\TestCase;

Expand All @@ -37,13 +38,13 @@
*/
class AccountMapperTest extends TestCase {

/**
* @var IDBConnection
*/
/** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */
protected $config;

/** @var IDBConnection */
protected $connection;
/**
* @var AccountMapper
*/

/** @var AccountMapper */
protected $mapper;

public static function setUpBeforeClass() {
Expand Down Expand Up @@ -76,8 +77,16 @@ public static function setUpBeforeClass() {

public function setUp() {
parent::setUp();

$this->config = $this->createMock(IConfig::class);

$this->connection = \OC::$server->getDatabaseConnection();
$this->mapper = new AccountMapper($this->connection, new AccountTermMapper($this->connection));

$this->mapper = new AccountMapper(
$this->config,
$this->connection,
new AccountTermMapper($this->connection)
);
}

public static function tearDownAfterClass () {
Expand All @@ -91,7 +100,6 @@ public static function tearDownAfterClass () {
public function testFindAll() {
$result = $this->mapper->find("testfind");
$this->assertEquals(4, count($result));

}

/**
Expand All @@ -101,7 +109,6 @@ public function testFindByUserId() {
$result = $this->mapper->find("testfind1");
$this->assertEquals(1, count($result));
$this->assertEquals("TestFind1", array_shift($result)->getUserId());

}

/**
Expand All @@ -111,7 +118,6 @@ public function testFindByDisplayName() {
$result = $this->mapper->find('test find 2');
$this->assertEquals(1, count($result));
$this->assertEquals("TestFind2", array_shift($result)->getUserId());

}

/**
Expand All @@ -121,7 +127,6 @@ public function testFindByEmail() {
$result= $this->mapper->find('[email protected]');
$this->assertEquals(1, count($result));
$this->assertEquals("TestFind3", array_shift($result)->getUserId());

}

/**
Expand All @@ -131,7 +136,6 @@ public function testFindBySearchTerm() {
$result = $this->mapper->find('term 4 b');
$this->assertEquals(1, count($result));
$this->assertEquals("TestFind4", array_shift($result)->getUserId());

}

/**
Expand All @@ -143,6 +147,5 @@ public function testFindLimitAndOffset() {
//results are ordered by display name
$this->assertEquals("TestFind3", array_shift($result)->getUserId());
$this->assertEquals("TestFind4", array_shift($result)->getUserId());

}
}