Merge pull request #28064 from owncloud/stable10-reenable-medial-search

Stable10 reenable medial search
owncloud · Jun 2, 2017 · 3ae53b6 · 3ae53b6
2 parents 4411934 + 5b85e52
commit 3ae53b6
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 26 deletions.
diff --git a/config/config.sample.php b/config/config.sample.php
@@ -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

diff --git a/core/Migrations/Version20170221114437.php b/core/Migrations/Version20170221114437.php
@@ -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;
 
@@ -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

diff --git 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
@@ -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;
 	}
 
@@ -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);
 
@@ -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);
 	}
@@ -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) {

diff --git 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()]);

diff --git a/tests/lib/User/AccountMapperTest.php b/tests/lib/User/AccountMapperTest.php
@@ -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;
 
@@ -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() {
@@ -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 () {
@@ -91,7 +100,6 @@ public static function tearDownAfterClass () {
 	public function testFindAll() {
 		$result = $this->mapper->find("testfind");
 		$this->assertEquals(4, count($result));
-
 	}
 
 	/**
@@ -101,7 +109,6 @@ public function testFindByUserId() {
 		$result = $this->mapper->find("testfind1");
 		$this->assertEquals(1, count($result));
 		$this->assertEquals("TestFind1", array_shift($result)->getUserId());
-
 	}
 
 	/**
@@ -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());
-
 	}
 
 	/**
@@ -121,7 +127,6 @@ public function testFindByEmail() {
 		$result= $this->mapper->find('test3@find.tld');
 		$this->assertEquals(1, count($result));
 		$this->assertEquals("TestFind3", array_shift($result)->getUserId());
-
 	}
 
 	/**
@@ -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());
-
 	}
 
 	/**
@@ -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());
-
 	}
 }