Merge pull request #14468 from owncloud/af-db-cleanup

Clean up AppFramework database layer

lib/private/appframework/db/db.php

@@ -21,31 +21,33 @@
  */
 namespace OC\AppFramework\Db;
 
-use \OCP\IDb;
-
+use OCP\IDb;
+use OCP\IDBConnection;
 
 /**
+ * @deprecated use IDBConnection directly, will be removed in ownCloud 10
  * Small Facade for being able to inject the database connection for tests
  */
 class Db implements IDb {
 	/**
-	 * @var \OCP\IDBConnection
+	 * @var IDBConnection
 	 */
 	protected $connection;
 
 	/**
-	 * @param \OCP\IDBConnection $connection
+	 * @param IDBConnection $connection
 	 */
-	public function __construct($connection) {
+	public function __construct(IDBConnection $connection) {
 		$this->connection = $connection;
 	}
 
 	/**
-	 * Used to abstract the owncloud database access away
+	 * Used to abstract the ownCloud database access away
 	 *
 	 * @param string $sql the sql query with ? placeholder for params
 	 * @param int $limit the maximum number of rows
 	 * @param int $offset from which row we want to start
+	 * @deprecated use prepare instead, will be removed in ownCloud 10
 	 * @return \OC_DB_StatementWrapper prepared SQL query
 	 */
 	public function prepareQuery($sql, $limit = null, $offset = null) {
@@ -58,6 +60,7 @@ class Db implements IDb {
 	/**
 	 * Used to get the id of the just inserted element
 	 *
+	 * @deprecated use lastInsertId instead, will be removed in ownCloud 10
 	 * @param string $tableName the name of the table where we inserted the item
 	 * @return int the id of the inserted element
 	 */
@@ -65,5 +68,179 @@ class Db implements IDb {
 		return $this->connection->lastInsertId($tableName);
 	}
 
+	/**
+	 * Used to abstract the ownCloud database access away
+	 * @param string $sql the sql query with ? placeholder for params
+	 * @param int $limit the maximum number of rows
+	 * @param int $offset from which row we want to start
+	 * @return \Doctrine\DBAL\Driver\Statement The prepared statement.
+	 */
+	public function prepare($sql, $limit=null, $offset=null) {
+		return $this->connection->prepare($sql, $limit, $offset);
+	}
+
+	/**
+	 * Executes an, optionally parameterized, SQL query.
+	 *
+	 * If the query is parameterized, a prepared statement is used.
+	 * If an SQLLogger is configured, the execution is logged.
+	 *
+	 * @param string $query The SQL query to execute.
+	 * @param string[] $params The parameters to bind to the query, if any.
+	 * @param array $types The types the previous parameters are in.
+	 * @return \Doctrine\DBAL\Driver\Statement The executed statement.
+	 */
+	public function executeQuery($query, array $params = array(), $types = array()) {
+		return $this->connection->executeQuery($query, $params, $types);
+	}
+
+	/**
+	 * Executes an SQL INSERT/UPDATE/DELETE query with the given parameters
+	 * and returns the number of affected rows.
+	 *
+	 * This method supports PDO binding types as well as DBAL mapping types.
+	 *
+	 * @param string $query The SQL query.
+	 * @param array $params The query parameters.
+	 * @param array $types The parameter types.
+	 * @return integer The number of affected rows.
+	 */
+	public function executeUpdate($query, array $params = array(), array $types = array()) {
+		return $this->connection->executeUpdate($query, $params, $types);
+	}
+
+	/**
+	 * Used to get the id of the just inserted element
+	 * @param string $table the name of the table where we inserted the item
+	 * @return int the id of the inserted element
+	 */
+	public function lastInsertId($table = null) {
+		return $this->connection->lastInsertId($table);
+	}
+
+	/**
+	 * Insert a row if a matching row doesn't exists.
+	 * @param string $table The table name (will replace *PREFIX*) to perform the replace on.
+	 * @param array $input
+	 * @throws \OC\HintException
+	 *
+	 * The input array if in the form:
+	 *
+	 * array ( 'id' => array ( 'value' => 6,
+	 *	'key' => true
+	 *	),
+	 *	'name' => array ('value' => 'Stoyan'),
+	 *	'family' => array ('value' => 'Stefanov'),
+	 *	'birth_date' => array ('value' => '1975-06-20')
+	 *	);
+	 * @return bool
+	 *
+	 */
+	public function insertIfNotExist($table, $input) {
+		return $this->connection->insertIfNotExist($table, $input);
+	}
+
+	/**
+	 * Start a transaction
+	 */
+	public function beginTransaction() {
+		$this->connection->beginTransaction();
+	}
+
+	/**
+	 * Commit the database changes done during a transaction that is in progress
+	 */
+	public function commit() {
+		$this->connection->commit();
+	}
+
+	/**
+	 * Rollback the database changes done during a transaction that is in progress
+	 */
+	public function rollBack() {
+		$this->connection->rollBack();
+	}
+
+	/**
+	 * Gets the error code and message as a string for logging
+	 * @return string
+	 */
+	public function getError() {
+		return $this->connection->getError();
+	}
+
+	/**
+	 * Fetch the SQLSTATE associated with the last database operation.
+	 *
+	 * @return integer The last error code.
+	 */
+	public function errorCode() {
+		return $this->connection->errorCode();
+	}
+
+	/**
+	 * Fetch extended error information associated with the last database operation.
+	 *
+	 * @return array The last error information.
+	 */
+	public function errorInfo() {
+		return $this->connection->errorInfo();
+	}
+
+	/**
+	 * Establishes the connection with the database.
+	 *
+	 * @return bool
+	 */
+	public function connect() {
+		return $this->connection->connect();
+	}
+
+	/**
+	 * Close the database connection
+	 */
+	public function close() {
+		$this->connection->close();
+	}
+
+	/**
+	 * Quotes a given input parameter.
+	 *
+	 * @param mixed $input Parameter to be quoted.
+	 * @param int $type Type of the parameter.
+	 * @return string The quoted parameter.
+	 */
+	public function quote($input, $type = \PDO::PARAM_STR) {
+		return $this->connection->quote($input, $type);
+	}
+
+	/**
+	 * Gets the DatabasePlatform instance that provides all the metadata about
+	 * the platform this driver connects to.
+	 *
+	 * @return \Doctrine\DBAL\Platforms\AbstractPlatform The database platform.
+	 */
+	public function getDatabasePlatform() {
+		return $this->connection->getDatabasePlatform();
+	}
+
+	/**
+	 * Drop a table from the database if it exists
+	 *
+	 * @param string $table table name without the prefix
+	 */
+	public function dropTable($table) {
+		$this->connection->dropTable($table);
+	}
+
+	/**
+	 * Check if a table exists
+	 *
+	 * @param string $table table name without the prefix
+	 * @return bool
+	 */
+	public function tableExists($table) {
+		return $this->connection->tableExists($table);
+	}
 
 }

lib/private/server.php

@@ -77,7 +77,7 @@ class Server extends SimpleContainer implements IServerContainer {
 			return new PreviewManager();
 		});
 		$this->registerService('TagMapper', function(Server $c) {
-			return new TagMapper($c->getDb());
+			return new TagMapper($c->getDatabaseConnection());
 		});
 		$this->registerService('TagManager', function (Server $c) {
 			$tagMapper = $c->query('TagMapper');
@@ -655,7 +655,7 @@ class Server extends SimpleContainer implements IServerContainer {
 
 	/**
 	 * Returns an instance of the db facade
-	 *
+	 * @deprecated use getDatabaseConnection, will be removed in ownCloud 10
 	 * @return \OCP\IDb
 	 */
 	function getDb() {

lib/private/tagging/tagmapper.php

@@ -22,7 +22,7 @@ namespace OC\Tagging;
 
 use \OCP\AppFramework\Db\Mapper,
     \OCP\AppFramework\Db\DoesNotExistException,
-    \OCP\IDb;
+    \OCP\IDBConnection;
 
 /**
  * Mapper for Tag entity
@@ -32,9 +32,9 @@ class TagMapper extends Mapper {
 	/**
 	* Constructor.
 	*
-	* @param IDb $db Instance of the Db abstraction layer.
+	* @param IDBConnection $db Instance of the Db abstraction layer.
 	*/
-	public function __construct(IDb $db) {
+	public function __construct(IDBConnection $db) {
 		parent::__construct($db, 'vcategory', 'OC\Tagging\Tag');
 	}
 

lib/public/appframework/db/mapper.php

@@ -22,7 +22,7 @@
  */
 namespace OCP\AppFramework\Db;
 
-use \OCP\IDb;
+use \OCP\IDBConnection;
 
 
 /**
@@ -36,12 +36,12 @@ abstract class Mapper {
 	protected $db;
 
 	/**
-	 * @param IDb $db Instance of the Db abstraction layer
+	 * @param IDBConnection $db Instance of the Db abstraction layer
 	 * @param string $tableName the name of the table. set this to allow entity
 	 * @param string $entityClass the name of the entity that the sql should be
 	 * mapped to queries without using sql
 	 */
-	public function __construct(IDb $db, $tableName, $entityClass=null){
+	public function __construct(IDBConnection $db, $tableName, $entityClass=null){
 		$this->db = $db;
 		$this->tableName = '*PREFIX*' . $tableName;
 
@@ -70,7 +70,8 @@ abstract class Mapper {
 	 */
 	public function delete(Entity $entity){
 		$sql = 'DELETE FROM `' . $this->tableName . '` WHERE `id` = ?';
-		$this->execute($sql, [$entity->getId()]);
+		$stmt = $this->execute($sql, [$entity->getId()]);
+		$stmt->closeCursor();
 		return $entity;
 	}
 
@@ -103,7 +104,7 @@ abstract class Mapper {
 				$values .= ',';
 			}
 
-			array_push($params, $entity->$getter());
+			$params[] = $entity->$getter();
 			$i++;
 
 		}
@@ -111,9 +112,11 @@ abstract class Mapper {
 		$sql = 'INSERT INTO `' . $this->tableName . '`(' .
 				$columns . ') VALUES(' . $values . ')';
 
-		$this->execute($sql, $params);
+		$stmt = $this->execute($sql, $params);
 
-		$entity->setId((int) $this->db->getInsertId($this->tableName));
+		$entity->setId((int) $this->db->lastInsertId($this->tableName));
+
+		$stmt->closeCursor();
 
 		return $entity;
 	}
@@ -162,15 +165,16 @@ abstract class Mapper {
 				$columns .= ',';
 			}
 
-			array_push($params, $entity->$getter());
+			$params[] = $entity->$getter();
 			$i++;
 		}
 
 		$sql = 'UPDATE `' . $this->tableName . '` SET ' .
 				$columns . ' WHERE `id` = ?';
-		array_push($params, $id);
+		$params[] = $id;
 
-		$this->execute($sql, $params);
+		$stmt = $this->execute($sql, $params);
+		$stmt->closeCursor();
 
 		return $entity;
 	}
@@ -185,7 +189,7 @@ abstract class Mapper {
 	 * @return \PDOStatement the database query result
 	 */
 	protected function execute($sql, array $params=[], $limit=null, $offset=null){
-		$query = $this->db->prepareQuery($sql, $limit, $offset);
+		$query = $this->db->prepare($sql, $limit, $offset);
 
 		$index = 1;  // bindParam is 1 indexed
 		foreach($params as $param) {
@@ -209,7 +213,9 @@ abstract class Mapper {
 			$index++;
 		}
 
-		return $query->execute();
+		$query->execute();
+
+		return $query;
 	}
 
 

lib/public/idb.php

@@ -24,7 +24,7 @@ namespace OCP;
 /**
  * Small Facade for being able to inject the database connection for tests
  */
-interface IDb {
+interface IDb extends IDBConnection {
 
 
     /**

lib/public/idbconnection.php

@@ -31,7 +31,7 @@ namespace OCP;
  */
 interface IDBConnection {
 	/**
-	 * Used to abstract the owncloud database access away
+	 * Used to abstract the ownCloud database access away
 	 * @param string $sql the sql query with ? placeholder for params
 	 * @param int $limit the maximum number of rows
 	 * @param int $offset from which row we want to start

lib/public/iservercontainer.php

@@ -144,6 +144,7 @@ interface IServerContainer {
 
 	/**
 	 * Returns an instance of the db facade
+	 * @deprecated use getDatabaseConnection, will be removed in ownCloud 10
 	 * @return \OCP\IDb
 	 */
 	function getDb();
@@ -255,7 +256,7 @@ interface IServerContainer {
 	 * @return \OCP\ICertificateManager
 	 */
 	function getCertificateManager($user = null);
-	
+
 	/**
 	 * Create a new event source
 	 *

tests/lib/appframework/db/mappertest.php

@@ -24,7 +24,7 @@
 
 namespace OCP\AppFramework\Db;
 
-use \OCP\IDb;
+use \OCP\IDBConnection;
 use Test\AppFramework\Db\MapperTestUtility;
 
 /**
@@ -42,7 +42,7 @@ class Example extends Entity {
 
 
 class ExampleMapper extends Mapper {
-	public function __construct(IDb $db){ parent::__construct($db, 'table'); }
+	public function __construct(IDBConnection $db){ parent::__construct($db, 'table'); }
 	public function find($table, $id){ return $this->findOneQuery($table, $id); }
 	public function findOneEntity($table, $id){ return $this->findEntity($table, $id); }
 	public function findAllEntities($table){ return $this->findEntities($table); }
@@ -137,7 +137,7 @@ class MapperTest extends MapperTestUtility {
 		$sql = 'DELETE FROM `*PREFIX*table` WHERE `id` = ?';
 		$params = array(2);
 
-		$this->setMapperResult($sql, $params);
+		$this->setMapperResult($sql, $params, [], null, null, true);
 		$entity = new Example();
 		$entity->setId($params[0]);
 
@@ -147,7 +147,7 @@ class MapperTest extends MapperTestUtility {
 
 	public function testCreate(){
 		$this->db->expects($this->once())
-			->method('getInsertId')
+			->method('lastInsertId')
 			->with($this->equalTo('*PREFIX*table'))
 			->will($this->returnValue(3));
 		$this->mapper = new ExampleMapper($this->db);
@@ -159,7 +159,7 @@ class MapperTest extends MapperTestUtility {
 		$entity->setPreName($params[0]);
 		$entity->setEmail($params[1]);
 
-		$this->setMapperResult($sql, $params);
+		$this->setMapperResult($sql, $params, [], null, null, true);
 
 		$this->mapper->insert($entity);
 	}
@@ -167,7 +167,7 @@ class MapperTest extends MapperTestUtility {
 
 	public function testCreateShouldReturnItemWithCorrectInsertId(){
 		$this->db->expects($this->once())
-			->method('getInsertId')
+			->method('lastInsertId')
 			->with($this->equalTo('*PREFIX*table'))
 			->will($this->returnValue(3));
 		$this->mapper = new ExampleMapper($this->db);
@@ -200,7 +200,7 @@ class MapperTest extends MapperTestUtility {
 		$entity->setEmail($params[1]);
 		$entity->setId($params[2]);
 
-		$this->setMapperResult($sql, $params);
+		$this->setMapperResult($sql, $params, [], null, null, true);
 
 		$this->mapper->update($entity);
 	}

tests/lib/appframework/db/mappertestutility.php

@@ -31,7 +31,6 @@ namespace Test\AppFramework\Db;
 abstract class MapperTestUtility extends \Test\TestCase {
 	protected $db;
 	private $query;
-	private $pdoResult;
 	private $queryAt;
 	private $prepareAt;
 	private $fetchAt;
@@ -46,19 +45,19 @@ abstract class MapperTestUtility extends \Test\TestCase {
 		parent::setUp();
 
 		$this->db = $this->getMockBuilder(
-			'\OCP\IDb')
+			'\OCP\IDBConnection')
 			->disableOriginalConstructor()
 			->getMock();
 
-		$this->query = $this->getMock('Query', array('execute', 'bindValue'));
-		$this->pdoResult = $this->getMock('Result', array('fetch', 'closeCursor'));
+		$this->query = $this->getMock('\PDOStatement');
 		$this->queryAt = 0;
 		$this->prepareAt = 0;
-		$this->iterators = array();
+		$this->iterators = [];
 		$this->fetchAt = 0;
 	}
 
 
+
 	/**
 	 * Create mocks and set expected results for database queries
 	 * @param string $sql the sql query that you expect to receive
@@ -70,13 +69,38 @@ abstract class MapperTestUtility extends \Test\TestCase {
 	 */
 	protected function setMapperResult($sql, $arguments=array(), $returnRows=array(),
 		$limit=null, $offset=null, $expectClose=false){
+		if($limit === null && $offset === null) {
+			$this->db->expects($this->at($this->prepareAt))
+				->method('prepare')
+				->with($this->equalTo($sql))
+				->will(($this->returnValue($this->query)));
+		} elseif($limit !== null && $offset === null) {
+			$this->db->expects($this->at($this->prepareAt))
+				->method('prepare')
+				->with($this->equalTo($sql), $this->equalTo($limit))
+				->will(($this->returnValue($this->query)));
+		} elseif($limit === null && $offset !== null) {
+			$this->db->expects($this->at($this->prepareAt))
+				->method('prepare')
+				->with($this->equalTo($sql),
+					$this->equalTo(null),
+					$this->equalTo($offset))
+				->will(($this->returnValue($this->query)));
+		} else  {
+			$this->db->expects($this->at($this->prepareAt))
+				->method('prepare')
+				->with($this->equalTo($sql),
+					$this->equalTo($limit),
+					$this->equalTo($offset))
+				->will(($this->returnValue($this->query)));
+		}
 
 		$this->iterators[] = new ArgumentIterator($returnRows);
 
 		$iterators = $this->iterators;
 		$fetchAt = $this->fetchAt;
 
-		$this->pdoResult->expects($this->any())
+		$this->query->expects($this->any())
 			->method('fetch')
 			->will($this->returnCallback(
 				function() use ($iterators, $fetchAt){
@@ -87,15 +111,11 @@ abstract class MapperTestUtility extends \Test\TestCase {
 						$fetchAt++;
 					}
 
+					$this->queryAt++;
+
 					return $result;
 				}
 			));
-		if ($expectClose) {
-			$closing = $this->once();
-		} else {
-			$closing = $this->any();
-		}
-		$this->pdoResult->expects($closing)->method('closeCursor');
 
 		$index = 1;
 		foreach($arguments as $argument) {
@@ -127,38 +147,23 @@ abstract class MapperTestUtility extends \Test\TestCase {
 
 		$this->query->expects($this->at($this->queryAt))
 			->method('execute')
-			->with()
-			->will($this->returnValue($this->pdoResult));
+			->will($this->returnCallback(function($sql, $p=null, $o=null, $s=null) {
+
+			}));
 		$this->queryAt++;
 
-		if($limit === null && $offset === null) {
-			$this->db->expects($this->at($this->prepareAt))
-				->method('prepareQuery')
-				->with($this->equalTo($sql))
-				->will(($this->returnValue($this->query)));
-		} elseif($limit !== null && $offset === null) {
-			$this->db->expects($this->at($this->prepareAt))
-				->method('prepareQuery')
-				->with($this->equalTo($sql), $this->equalTo($limit))
-				->will(($this->returnValue($this->query)));
-		} elseif($limit === null && $offset !== null) {
-			$this->db->expects($this->at($this->prepareAt))
-				->method('prepareQuery')
-				->with($this->equalTo($sql),
-					$this->equalTo(null),
-					$this->equalTo($offset))
-				->will(($this->returnValue($this->query)));
-		} else  {
-			$this->db->expects($this->at($this->prepareAt))
-				->method('prepareQuery')
-				->with($this->equalTo($sql),
-					$this->equalTo($limit),
-					$this->equalTo($offset))
-				->will(($this->returnValue($this->query)));
+
+
+		if ($expectClose) {
+			$closing = $this->at($this->queryAt);
+		} else {
+			$closing = $this->any();
 		}
+		$this->query->expects($closing)->method('closeCursor');
+		$this->queryAt++;
+
 		$this->prepareAt++;
 		$this->fetchAt++;
-
 	}
 
 

tests/lib/tags.php

@@ -49,7 +49,7 @@ class Test_Tags extends \Test\TestCase {
 			->will($this->returnValue($this->user));
 
 		$this->objectType = $this->getUniqueID('type_');
-		$this->tagMapper = new OC\Tagging\TagMapper(\OC::$server->getDb());
+		$this->tagMapper = new OC\Tagging\TagMapper(\OC::$server->getDatabaseConnection());
 		$this->tagMgr = new OC\TagManager($this->tagMapper, $this->userSession);
 
 	}