remove optional medial search, update tech debt todos

owncloud · May 18, 2017 · d9fc93c · d9fc93c
1 parent 148d443
commit d9fc93c
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 27 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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()]);