diff --git a/lib/private/db/statementwrapper.php b/lib/private/db/statementwrapper.php index a85c0167e0..2f833a27a6 100644 --- a/lib/private/db/statementwrapper.php +++ b/lib/private/db/statementwrapper.php @@ -69,6 +69,7 @@ class OC_DB_StatementWrapper { return false; } if ($this->isManipulation) { + $this->statement->closeCursor(); return $this->statement->rowCount(); } else { return $this; diff --git a/lib/public/appframework/db/mapper.php b/lib/public/appframework/db/mapper.php index 03f0fe3aa7..fd3dc9b747 100644 --- a/lib/public/appframework/db/mapper.php +++ b/lib/public/appframework/db/mapper.php @@ -41,7 +41,7 @@ abstract class Mapper { /** * @param IDb $db Instance of the Db abstraction layer - * @param string $tableName the name of the table. set this to allow entity + * @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 */ @@ -70,10 +70,12 @@ abstract class Mapper { /** * Deletes an entity from the table * @param Entity $entity the entity that should be deleted + * @return Entity the deleted entity */ public function delete(Entity $entity){ $sql = 'DELETE FROM `' . $this->tableName . '` WHERE `id` = ?'; - $this->execute($sql, array($entity->getId())); + $this->execute($sql, [$entity->getId()]); + return $entity; } @@ -88,14 +90,14 @@ abstract class Mapper { $properties = $entity->getUpdatedFields(); $values = ''; $columns = ''; - $params = array(); + $params = []; // build the fields $i = 0; foreach($properties as $property => $updated) { $column = $entity->propertyToColumn($property); $getter = 'get' . ucfirst($property); - + $columns .= '`' . $column . '`'; $values .= '?'; @@ -112,10 +114,11 @@ abstract class Mapper { $sql = 'INSERT INTO `' . $this->tableName . '`(' . $columns . ') VALUES(' . $values . ')'; - + $this->execute($sql, $params); $entity->setId((int) $this->db->getInsertId($this->tableName)); + return $entity; } @@ -147,7 +150,7 @@ abstract class Mapper { unset($properties['id']); $columns = ''; - $params = array(); + $params = []; // build the fields $i = 0; @@ -155,7 +158,7 @@ abstract class Mapper { $column = $entity->propertyToColumn($property); $getter = 'get' . ucfirst($property); - + $columns .= '`' . $column . '` = ?'; // only append colon if there are more entries @@ -167,7 +170,7 @@ abstract class Mapper { $i++; } - $sql = 'UPDATE `' . $this->tableName . '` SET ' . + $sql = 'UPDATE `' . $this->tableName . '` SET ' . $columns . ' WHERE `id` = ?'; array_push($params, $id); @@ -185,7 +188,7 @@ abstract class Mapper { * @param int $offset from which row we want to start * @return \PDOStatement the database query result */ - protected function execute($sql, array $params=array(), $limit=null, $offset=null){ + protected function execute($sql, array $params=[], $limit=null, $offset=null){ $query = $this->db->prepareQuery($sql, $limit, $offset); $index = 1; // bindParam is 1 indexed @@ -199,12 +202,12 @@ abstract class Mapper { case 'boolean': $pdoConstant = \PDO::PARAM_BOOL; break; - + default: $pdoConstant = \PDO::PARAM_STR; break; } - + $query->bindValue($index, $param, $pdoConstant); $index++; @@ -226,14 +229,16 @@ abstract class Mapper { * @throws MultipleObjectsReturnedException if more than one item exist * @return array the result as row */ - protected function findOneQuery($sql, array $params=array(), $limit=null, $offset=null){ - $result = $this->execute($sql, $params, $limit, $offset); - $row = $result->fetch(); + protected function findOneQuery($sql, array $params=[], $limit=null, $offset=null){ + $stmt = $this->execute($sql, $params, $limit, $offset); + $row = $stmt->fetch(); if($row === false || $row === null){ + $stmt->closeCursor(); throw new DoesNotExistException('No matching entry found'); } - $row2 = $result->fetch(); + $row2 = $stmt->fetch(); + $stmt->closeCursor(); //MDB2 returns null, PDO and doctrine false when no row is available if( ! ($row2 === false || $row2 === null )) { throw new MultipleObjectsReturnedException('More than one result'); @@ -262,15 +267,17 @@ abstract class Mapper { * @param int $offset from which row we want to start * @return array all fetched entities */ - protected function findEntities($sql, array $params=array(), $limit=null, $offset=null) { - $result = $this->execute($sql, $params, $limit, $offset); + protected function findEntities($sql, array $params=[], $limit=null, $offset=null) { + $stmt = $this->execute($sql, $params, $limit, $offset); - $entities = array(); - - while($row = $result->fetch()){ + $entities = []; + + while($row = $stmt->fetch()){ $entities[] = $this->mapRowToEntity($row); } + $stmt->closeCursor(); + return $entities; } @@ -286,7 +293,7 @@ abstract class Mapper { * @throws MultipleObjectsReturnedException if more than one item exist * @return Entity the entity */ - protected function findEntity($sql, array $params=array(), $limit=null, $offset=null){ + protected function findEntity($sql, array $params=[], $limit=null, $offset=null){ return $this->mapRowToEntity($this->findOneQuery($sql, $params, $limit, $offset)); } diff --git a/tests/lib/appframework/db/mappertest.php b/tests/lib/appframework/db/mappertest.php index 6ad8cd86bf..9cbc01883e 100644 --- a/tests/lib/appframework/db/mappertest.php +++ b/tests/lib/appframework/db/mappertest.php @@ -74,7 +74,7 @@ class MapperTest extends MapperTestUtility { $rows = array( array('hi') ); - $this->setMapperResult($sql, $params, $rows); + $this->setMapperResult($sql, $params, $rows); $this->mapper->find($sql, $params); } @@ -84,7 +84,7 @@ class MapperTest extends MapperTestUtility { $rows = array( array('pre_name' => 'hi') ); - $this->setMapperResult($sql, $params, $rows); + $this->setMapperResult($sql, $params, $rows, null, null, true); $this->mapper->findOneEntity($sql, $params); } @@ -92,7 +92,7 @@ class MapperTest extends MapperTestUtility { $sql = 'hi'; $params = array('jo'); $rows = array(); - $this->setMapperResult($sql, $params, $rows); + $this->setMapperResult($sql, $params, $rows); $this->setExpectedException( '\OCP\AppFramework\Db\DoesNotExistException'); $this->mapper->find($sql, $params); @@ -102,7 +102,7 @@ class MapperTest extends MapperTestUtility { $sql = 'hi'; $params = array('jo'); $rows = array(); - $this->setMapperResult($sql, $params, $rows); + $this->setMapperResult($sql, $params, $rows, null, null, true); $this->setExpectedException( '\OCP\AppFramework\Db\DoesNotExistException'); $this->mapper->findOneEntity($sql, $params); @@ -114,7 +114,7 @@ class MapperTest extends MapperTestUtility { $rows = array( array('jo'), array('ho') ); - $this->setMapperResult($sql, $params, $rows); + $this->setMapperResult($sql, $params, $rows, null, null, true); $this->setExpectedException( '\OCP\AppFramework\Db\MultipleObjectsReturnedException'); $this->mapper->find($sql, $params); @@ -126,7 +126,7 @@ class MapperTest extends MapperTestUtility { $rows = array( array('jo'), array('ho') ); - $this->setMapperResult($sql, $params, $rows); + $this->setMapperResult($sql, $params, $rows, null, null, true); $this->setExpectedException( '\OCP\AppFramework\Db\MultipleObjectsReturnedException'); $this->mapper->findOneEntity($sql, $params); @@ -249,7 +249,7 @@ class MapperTest extends MapperTestUtility { $entity = new Example(); $entity->setPreName('hi'); $entity->resetUpdatedFields(); - $this->setMapperResult($sql, array(), $rows); + $this->setMapperResult($sql, array(), $rows, null, null, true); $result = $this->mapper->findAllEntities($sql); $this->assertEquals(array($entity), $result); } diff --git a/tests/lib/appframework/db/mappertestutility.php b/tests/lib/appframework/db/mappertestutility.php index ad7a67a96b..0053b2c682 100644 --- a/tests/lib/appframework/db/mappertestutility.php +++ b/tests/lib/appframework/db/mappertestutility.php @@ -36,7 +36,7 @@ abstract class MapperTestUtility extends \Test\TestCase { private $prepareAt; private $fetchAt; private $iterators; - + /** * Run this function before the actual test to either set or initialize the @@ -51,7 +51,7 @@ abstract class MapperTestUtility extends \Test\TestCase { ->getMock(); $this->query = $this->getMock('Query', array('execute', 'bindValue')); - $this->pdoResult = $this->getMock('Result', array('fetch')); + $this->pdoResult = $this->getMock('Result', array('fetch', 'closeCursor')); $this->queryAt = 0; $this->prepareAt = 0; $this->iterators = array(); @@ -69,7 +69,7 @@ abstract class MapperTestUtility extends \Test\TestCase { * will be called on the result */ protected function setMapperResult($sql, $arguments=array(), $returnRows=array(), - $limit=null, $offset=null){ + $limit=null, $offset=null, $expectClose=false){ $this->iterators[] = new ArgumentIterator($returnRows); @@ -88,8 +88,14 @@ abstract class MapperTestUtility extends \Test\TestCase { } return $result; - } + } )); + if ($expectClose) { + $closing = $this->once(); + } else { + $closing = $this->any(); + } + $this->pdoResult->expects($closing)->method('closeCursor'); $index = 1; foreach($arguments as $argument) { @@ -105,7 +111,7 @@ abstract class MapperTestUtility extends \Test\TestCase { case 'boolean': $pdoConstant = \PDO::PARAM_BOOL; break; - + default: $pdoConstant = \PDO::PARAM_STR; break; @@ -138,14 +144,14 @@ abstract class MapperTestUtility extends \Test\TestCase { } elseif($limit === null && $offset !== null) { $this->db->expects($this->at($this->prepareAt)) ->method('prepareQuery') - ->with($this->equalTo($sql), + ->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), + ->with($this->equalTo($sql), $this->equalTo($limit), $this->equalTo($offset)) ->will(($this->returnValue($this->query))); @@ -162,11 +168,11 @@ abstract class MapperTestUtility extends \Test\TestCase { class ArgumentIterator { private $arguments; - + public function __construct($arguments){ $this->arguments = $arguments; } - + public function next(){ $result = array_shift($this->arguments); if($result === null){