Merge pull request #32069 from nextcloud/cleanup/allconfig-sql

Cleanup AllConfig

lib/private/AllConfig.php

@@ -33,18 +33,17 @@
 namespace OC;
 
 use OC\Cache\CappedMemoryCache;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\PreConditionNotMetException;
 
 /**
  * Class to combine all the configuration options ownCloud offers
  */
-class AllConfig implements \OCP\IConfig {
-	/** @var SystemConfig */
-	private $systemConfig;
-
-	/** @var IDBConnection */
-	private $connection;
+class AllConfig implements IConfig {
+	private SystemConfig $systemConfig;
+	private ?IDBConnection $connection = null;
 
 	/**
 	 * 3 dimensional array with the following structure:
@@ -66,11 +65,8 @@ class AllConfig implements \OCP\IConfig {
 	 *
 	 * @var CappedMemoryCache $userCache
 	 */
-	private $userCache;
+	private CappedMemoryCache $userCache;
 
-	/**
-	 * @param SystemConfig $systemConfig
-	 */
 	public function __construct(SystemConfig $systemConfig) {
 		$this->userCache = new CappedMemoryCache();
 		$this->systemConfig = $systemConfig;
@@ -91,7 +87,7 @@ class AllConfig implements \OCP\IConfig {
 	 */
 	private function fixDIInit() {
 		if ($this->connection === null) {
-			$this->connection = \OC::$server->getDatabaseConnection();
+			$this->connection = \OC::$server->get(IDBConnection::class);
 		}
 	}
 
@@ -195,7 +191,7 @@ class AllConfig implements \OCP\IConfig {
 	 * @return string[] the keys stored for the app
 	 */
 	public function getAppKeys($appName) {
-		return \OC::$server->query(\OC\AppConfig::class)->getKeys($appName);
+		return \OC::$server->get(AppConfig::class)->getKeys($appName);
 	}
 
 	/**
@@ -206,7 +202,7 @@ class AllConfig implements \OCP\IConfig {
 	 * @param string|float|int $value the value that should be stored
 	 */
 	public function setAppValue($appName, $key, $value) {
-		\OC::$server->query(\OC\AppConfig::class)->setValue($appName, $key, $value);
+		\OC::$server->get(AppConfig::class)->setValue($appName, $key, $value);
 	}
 
 	/**
@@ -218,7 +214,7 @@ class AllConfig implements \OCP\IConfig {
 	 * @return string the saved value
 	 */
 	public function getAppValue($appName, $key, $default = '') {
-		return \OC::$server->query(\OC\AppConfig::class)->getValue($appName, $key, $default);
+		return \OC::$server->get(AppConfig::class)->getValue($appName, $key, $default);
 	}
 
 	/**
@@ -228,7 +224,7 @@ class AllConfig implements \OCP\IConfig {
 	 * @param string $key the key of the value, under which it was saved
 	 */
 	public function deleteAppValue($appName, $key) {
-		\OC::$server->query(\OC\AppConfig::class)->deleteKey($appName, $key);
+		\OC::$server->get(AppConfig::class)->deleteKey($appName, $key);
 	}
 
 	/**
@@ -237,7 +233,7 @@ class AllConfig implements \OCP\IConfig {
 	 * @param string $appName the appName the configs are stored under
 	 */
 	public function deleteAppValues($appName) {
-		\OC::$server->query(\OC\AppConfig::class)->deleteApp($appName);
+		\OC::$server->get(AppConfig::class)->deleteApp($appName);
 	}
 
 
@@ -278,7 +274,7 @@ class AllConfig implements \OCP\IConfig {
 					->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId)))
 					->andWhere($qb->expr()->eq('appid', $qb->createNamedParameter($appName)))
 					->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter($key)));
-				$qb->execute();
+				$qb->executeStatement();
 
 				$this->userCache[$userId][$appName][$key] = (string)$value;
 				return;
@@ -354,9 +350,12 @@ class AllConfig implements \OCP\IConfig {
 		// TODO - FIXME
 		$this->fixDIInit();
 
-		$sql = 'DELETE FROM `*PREFIX*preferences` '.
-				'WHERE `userid` = ? AND `appid` = ? AND `configkey` = ?';
-		$this->connection->executeUpdate($sql, [$userId, $appName, $key]);
+		$qb = $this->connection->getQueryBuilder();
+		$qb->delete('preferences')
+			->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
+			->where($qb->expr()->eq('appid', $qb->createNamedParameter($appName, IQueryBuilder::PARAM_STR)))
+			->where($qb->expr()->eq('configkey', $qb->createNamedParameter($key, IQueryBuilder::PARAM_STR)))
+			->executeStatement();
 
 		if (isset($this->userCache[$userId][$appName])) {
 			unset($this->userCache[$userId][$appName][$key]);
@@ -371,10 +370,10 @@ class AllConfig implements \OCP\IConfig {
 	public function deleteAllUserValues($userId) {
 		// TODO - FIXME
 		$this->fixDIInit();
-
-		$sql = 'DELETE FROM `*PREFIX*preferences` '.
-			'WHERE `userid` = ?';
-		$this->connection->executeUpdate($sql, [$userId]);
+		$qb = $this->connection->getQueryBuilder();
+		$qb->delete('preferences')
+			->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
+			->executeStatement();
 
 		unset($this->userCache[$userId]);
 	}
@@ -388,9 +387,10 @@ class AllConfig implements \OCP\IConfig {
 		// TODO - FIXME
 		$this->fixDIInit();
 
-		$sql = 'DELETE FROM `*PREFIX*preferences` '.
-				'WHERE `appid` = ?';
-		$this->connection->executeUpdate($sql, [$appName]);
+		$qb = $this->connection->getQueryBuilder();
+		$qb->delete('preferences')
+			->where($qb->expr()->eq('appid', $qb->createNamedParameter($appName, IQueryBuilder::PARAM_STR)))
+			->executeStatement();
 
 		foreach ($this->userCache as &$userCache) {
 			unset($userCache[$appName]);
@@ -420,8 +420,12 @@ class AllConfig implements \OCP\IConfig {
 		$this->fixDIInit();
 
 		$data = [];
-		$query = 'SELECT `appid`, `configkey`, `configvalue` FROM `*PREFIX*preferences` WHERE `userid` = ?';
-		$result = $this->connection->executeQuery($query, [$userId]);
+
+		$qb = $this->connection->getQueryBuilder();
+		$result = $qb->select('appid', 'configkey', 'configvalue')
+			->from('preferences')
+			->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
+			->executeQuery();
 		while ($row = $result->fetch()) {
 			$appId = $row['appid'];
 			if (!isset($data[$appId])) {
@@ -450,22 +454,20 @@ class AllConfig implements \OCP\IConfig {
 		}
 
 		$chunkedUsers = array_chunk($userIds, 50, true);
-		$placeholders50 = implode(',', array_fill(0, 50, '?'));
+
+		$qb = $this->connection->getQueryBuilder();
+		$qb->select('userid', 'configvalue')
+			->from('preferences')
+			->where($qb->expr()->eq('appid', $qb->createParameter('appName')))
+			->andWhere($qb->expr()->eq('configkey', $qb->createParameter('configKey')))
+			->andWhere($qb->expr()->in('userid', $qb->createParameter('userIds')));
 
 		$userValues = [];
 		foreach ($chunkedUsers as $chunk) {
-			$queryParams = $chunk;
-			// create [$app, $key, $chunkedUsers]
-			array_unshift($queryParams, $key);
-			array_unshift($queryParams, $appName);
-
-			$placeholders = (count($chunk) === 50) ? $placeholders50 :  implode(',', array_fill(0, count($chunk), '?'));
-
-			$query = 'SELECT `userid`, `configvalue` ' .
-						'FROM `*PREFIX*preferences` ' .
-						'WHERE `appid` = ? AND `configkey` = ? ' .
-						'AND `userid` IN (' . $placeholders . ')';
-			$result = $this->connection->executeQuery($query, $queryParams);
+			$qb->setParameter('appName', $appName, IQueryBuilder::PARAM_STR);
+			$qb->setParameter('configKey', $key, IQueryBuilder::PARAM_STR);
+			$qb->setParameter('userIds', $chunk, IQueryBuilder::PARAM_STR_ARRAY);
+			$result = $qb->executeQuery();
 
 			while ($row = $result->fetch()) {
 				$userValues[$row['userid']] = $row['configvalue'];
@@ -487,19 +489,16 @@ class AllConfig implements \OCP\IConfig {
 		// TODO - FIXME
 		$this->fixDIInit();
 
-		$sql = 'SELECT `userid` FROM `*PREFIX*preferences` ' .
-				'WHERE `appid` = ? AND `configkey` = ? ';
-
-		if ($this->getSystemValue('dbtype', 'sqlite') === 'oci') {
-			//oracle hack: need to explicitly cast CLOB to CHAR for comparison
-			$sql .= 'AND to_char(`configvalue`) = ?';
-		} else {
-			$sql .= 'AND `configvalue` = ?';
-		}
-
-		$sql .= ' ORDER BY `userid`';
-
-		$result = $this->connection->executeQuery($sql, [$appName, $key, $value]);
+		$qb = $this->connection->getQueryBuilder();
+		$result = $qb->select('userid')
+			->from('preferences')
+			->where($qb->expr()->eq('appid', $qb->createNamedParameter($appName, IQueryBuilder::PARAM_STR)))
+			->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter($key, IQueryBuilder::PARAM_STR)))
+			->andWhere($qb->expr()->eq(
+				$qb->expr()->castColumn('configvalue', IQueryBuilder::PARAM_STR),
+				$qb->createNamedParameter($value, IQueryBuilder::PARAM_STR))
+			)->orderBy('userid')
+			->executeQuery();
 
 		$userIDs = [];
 		while ($row = $result->fetch()) {
@@ -525,20 +524,16 @@ class AllConfig implements \OCP\IConfig {
 			// Email address is always stored lowercase in the database
 			return $this->getUsersForUserValue($appName, $key, strtolower($value));
 		}
-
-		$sql = 'SELECT `userid` FROM `*PREFIX*preferences` ' .
-			'WHERE `appid` = ? AND `configkey` = ? ';
-
-		if ($this->getSystemValue('dbtype', 'sqlite') === 'oci') {
-			//oracle hack: need to explicitly cast CLOB to CHAR for comparison
-			$sql .= 'AND LOWER(to_char(`configvalue`)) = ?';
-		} else {
-			$sql .= 'AND LOWER(`configvalue`) = ?';
-		}
-
-		$sql .= ' ORDER BY `userid`';
-
-		$result = $this->connection->executeQuery($sql, [$appName, $key, strtolower($value)]);
+		$qb = $this->connection->getQueryBuilder();
+		$result = $qb->select('userid')
+			->from('preferences')
+			->where($qb->expr()->eq('appid', $qb->createNamedParameter($appName, IQueryBuilder::PARAM_STR)))
+			->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter($key, IQueryBuilder::PARAM_STR)))
+			->andWhere($qb->expr()->eq(
+				$qb->func()->lower($qb->expr()->castColumn('configvalue', IQueryBuilder::PARAM_STR)),
+				$qb->createNamedParameter(strtolower($value), IQueryBuilder::PARAM_STR))
+			)->orderBy('userid')
+			->executeQuery();
 
 		$userIDs = [];
 		while ($row = $result->fetch()) {

tests/lib/AllConfigTest.php

@@ -409,11 +409,6 @@ class AllConfigTest extends \Test\TestCase {
 		$systemConfig = $this->getMockBuilder('\OC\SystemConfig')
 			->disableOriginalConstructor()
 			->getMock();
-		$systemConfig->expects($this->once())
-			->method('getValue')
-			->with($this->equalTo('dbtype'),
-				$this->equalTo('sqlite'))
-			->willReturn(\OC::$server->getConfig()->getSystemValue('dbtype', 'sqlite'));
 		$config = $this->getConfig($systemConfig);
 
 		// preparation - add something to the database
@@ -443,10 +438,6 @@ class AllConfigTest extends \Test\TestCase {
 	public function testGetUsersForUserValueCaseInsensitive() {
 		// mock the check for the database to run the correct SQL statements for each database type
 		$systemConfig = $this->createMock(SystemConfig::class);
-		$systemConfig->expects($this->once())
-			->method('getValue')
-			->with($this->equalTo('dbtype'), $this->equalTo('sqlite'))
-			->willReturn(\OC::$server->getConfig()->getSystemValue('dbtype', 'sqlite'));
 		$config = $this->getConfig($systemConfig);
 
 		$config->setUserValue('user1', 'myApp', 'myKey', 'test123');