From 345e68cafac4db880ef5cf41f714cab9599b1f8a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 18 Dec 2015 09:47:54 +0100 Subject: [PATCH 1/2] Use the query builder for the joblist queries --- lib/private/backgroundjob/joblist.php | 123 ++++++++++++++++++-------- tests/lib/backgroundjob/joblist.php | 13 +-- 2 files changed, 92 insertions(+), 44 deletions(-) diff --git a/lib/private/backgroundjob/joblist.php b/lib/private/backgroundjob/joblist.php index 03c9180ddb0..226564e2175 100644 --- a/lib/private/backgroundjob/joblist.php +++ b/lib/private/backgroundjob/joblist.php @@ -28,22 +28,20 @@ use OCP\BackgroundJob\IJobList; use OCP\AutoloadNotAllowedException; class JobList implements IJobList { - /** - * @var \OCP\IDBConnection - */ - private $conn; + /** @var \OCP\IDBConnection */ + protected $connection; /** * @var \OCP\IConfig $config */ - private $config; + protected $config; /** - * @param \OCP\IDBConnection $conn + * @param \OCP\IDBConnection $connection * @param \OCP\IConfig $config */ - public function __construct($conn, $config) { - $this->conn = $conn; + public function __construct($connection, $config) { + $this->connection = $connection; $this->config = $config; } @@ -58,12 +56,20 @@ class JobList implements IJobList { } else { $class = $job; } + $argument = json_encode($argument); if (strlen($argument) > 4000) { throw new \InvalidArgumentException('Background job arguments can\'t exceed 4000 characters (json encoded)'); } - $query = $this->conn->prepare('INSERT INTO `*PREFIX*jobs`(`class`, `argument`, `last_run`) VALUES(?, ?, 0)'); - $query->execute(array($class, $argument)); + + $query = $this->connection->getQueryBuilder(); + $query->insert('jobs') + ->values([ + 'class' => $query->createNamedParameter($class), + 'argument' => $query->createNamedParameter($argument), + 'last_run' => $query->createNamedParameter(0, \PDO::PARAM_INT), + ]); + $query->execute(); } } @@ -77,19 +83,25 @@ class JobList implements IJobList { } else { $class = $job; } + + $query = $this->connection->getQueryBuilder(); + $query->delete('jobs') + ->where($query->expr()->eq('class', $query->createNamedParameter($class))); if (!is_null($argument)) { $argument = json_encode($argument); - $query = $this->conn->prepare('DELETE FROM `*PREFIX*jobs` WHERE `class` = ? AND `argument` = ?'); - $query->execute(array($class, $argument)); - } else { - $query = $this->conn->prepare('DELETE FROM `*PREFIX*jobs` WHERE `class` = ?'); - $query->execute(array($class)); + $query->andWhere($query->expr()->eq('argument', $query->createNamedParameter($argument))); } + $query->execute(); } + /** + * @param int $id + */ protected function removeById($id) { - $query = $this->conn->prepare('DELETE FROM `*PREFIX*jobs` WHERE `id` = ?'); - $query->execute([$id]); + $query = $this->connection->getQueryBuilder(); + $query->delete('jobs') + ->where($query->expr()->eq('id', $query->createNamedParameter($id, \PDO::PARAM_INT))); + $query->execute(); } /** @@ -106,9 +118,19 @@ class JobList implements IJobList { $class = $job; } $argument = json_encode($argument); - $query = $this->conn->prepare('SELECT `id` FROM `*PREFIX*jobs` WHERE `class` = ? AND `argument` = ?'); - $query->execute(array($class, $argument)); - return (bool)$query->fetch(); + + $query = $this->connection->getQueryBuilder(); + $query->select('id') + ->from('jobs') + ->where($query->expr()->eq('class', $query->createNamedParameter($class))) + ->andWhere($query->expr()->eq('argument', $query->createNamedParameter($argument))) + ->setMaxResults(1); + + $result = $query->execute(); + $row = $result->fetch(); + $result->closeCursor(); + + return (bool) $row; } /** @@ -117,15 +139,20 @@ class JobList implements IJobList { * @return Job[] */ public function getAll() { - $query = $this->conn->prepare('SELECT `id`, `class`, `last_run`, `argument` FROM `*PREFIX*jobs`'); - $query->execute(); - $jobs = array(); - while ($row = $query->fetch()) { + $query = $this->connection->getQueryBuilder(); + $query->select('*') + ->from('jobs'); + $result = $query->execute(); + + $jobs = []; + while ($row = $result->fetch()) { $job = $this->buildJob($row); if ($job) { $jobs[] = $job; } } + $result->closeCursor(); + return $jobs; } @@ -136,22 +163,39 @@ class JobList implements IJobList { */ public function getNext() { $lastId = $this->getLastJob(); - $query = $this->conn->prepare('SELECT `id`, `class`, `last_run`, `argument` FROM `*PREFIX*jobs` WHERE `id` > ? ORDER BY `id` ASC', 1); - $query->execute(array($lastId)); - if ($row = $query->fetch()) { + + $query = $this->connection->getQueryBuilder(); + $query->select('*') + ->from('jobs') + ->where($query->expr()->gt('id', $query->createNamedParameter($lastId, \PDO::PARAM_INT))) + ->orderBy('id', 'ASC') + ->setMaxResults(1); + $result = $query->execute(); + $row = $result->fetch(); + $result->closeCursor(); + + if ($row) { $jobId = $row['id']; $job = $this->buildJob($row); } else { //begin at the start of the queue - $query = $this->conn->prepare('SELECT `id`, `class`, `last_run`, `argument` FROM `*PREFIX*jobs` ORDER BY `id` ASC', 1); - $query->execute(); - if ($row = $query->fetch()) { + $query = $this->connection->getQueryBuilder(); + $query->select('*') + ->from('jobs') + ->orderBy('id', 'ASC') + ->setMaxResults(1); + $result = $query->execute(); + $row = $result->fetch(); + $result->closeCursor(); + + if ($row) { $jobId = $row['id']; $job = $this->buildJob($row); } else { return null; //empty job list } } + if (is_null($job)) { $this->removeById($jobId); return $this->getNext(); @@ -165,9 +209,15 @@ class JobList implements IJobList { * @return Job|null */ public function getById($id) { - $query = $this->conn->prepare('SELECT `id`, `class`, `last_run`, `argument` FROM `*PREFIX*jobs` WHERE `id` = ?'); - $query->execute(array($id)); - if ($row = $query->fetch()) { + $query = $this->connection->getQueryBuilder(); + $query->select('*') + ->from('jobs') + ->where($query->expr()->eq('id', $query->createNamedParameter($id, \PDO::PARAM_INT))); + $result = $query->execute(); + $row = $result->fetch(); + $result->closeCursor(); + + if ($row) { return $this->buildJob($row); } else { return null; @@ -225,7 +275,10 @@ class JobList implements IJobList { * @param Job $job */ public function setLastRun($job) { - $query = $this->conn->prepare('UPDATE `*PREFIX*jobs` SET `last_run` = ? WHERE `id` = ?'); - $query->execute(array(time(), $job->getId())); + $query = $this->connection->getQueryBuilder(); + $query->update('jobs') + ->set('last_run', $query->createNamedParameter(time(), \PDO::PARAM_INT)) + ->where($query->expr()->eq('id', $query->createNamedParameter($job->getId(), \PDO::PARAM_INT))); + $query->execute(); } } diff --git a/tests/lib/backgroundjob/joblist.php b/tests/lib/backgroundjob/joblist.php index 73b3255c079..c0796d8253a 100644 --- a/tests/lib/backgroundjob/joblist.php +++ b/tests/lib/backgroundjob/joblist.php @@ -15,26 +15,21 @@ use Test\TestCase; * Class JobList * * @group DB - * * @package Test\BackgroundJob */ class JobList extends TestCase { - /** - * @var \OC\BackgroundJob\JobList - */ + /** @var \OC\BackgroundJob\JobList */ protected $instance; - /** - * @var \OCP\IConfig | \PHPUnit_Framework_MockObject_MockObject $config - */ + /** @var \OCP\IConfig|\PHPUnit_Framework_MockObject_MockObject */ protected $config; protected function setUp() { parent::setUp(); - $conn = \OC::$server->getDatabaseConnection(); + $connection = \OC::$server->getDatabaseConnection(); $this->config = $this->getMock('\OCP\IConfig'); - $this->instance = new \OC\BackgroundJob\JobList($conn, $this->config); + $this->instance = new \OC\BackgroundJob\JobList($connection, $this->config); } protected function getAllSorted() { From 0f836cfe9ea0e8fb1267f55ed2b65747c7ec318c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 18 Dec 2015 10:04:30 +0100 Subject: [PATCH 2/2] Make sure the interface and the implementation match --- lib/private/backgroundjob/joblist.php | 29 ++++++++++++++------------- lib/public/backgroundjob/ijoblist.php | 8 ++------ 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/lib/private/backgroundjob/joblist.php b/lib/private/backgroundjob/joblist.php index 226564e2175..75c205833fe 100644 --- a/lib/private/backgroundjob/joblist.php +++ b/lib/private/backgroundjob/joblist.php @@ -24,6 +24,7 @@ namespace OC\BackgroundJob; +use OCP\BackgroundJob\IJob; use OCP\BackgroundJob\IJobList; use OCP\AutoloadNotAllowedException; @@ -46,12 +47,12 @@ class JobList implements IJobList { } /** - * @param Job|string $job + * @param IJob|string $job * @param mixed $argument */ public function add($job, $argument = null) { if (!$this->has($job, $argument)) { - if ($job instanceof Job) { + if ($job instanceof IJob) { $class = get_class($job); } else { $class = $job; @@ -74,11 +75,11 @@ class JobList implements IJobList { } /** - * @param Job|string $job + * @param IJob|string $job * @param mixed $argument */ public function remove($job, $argument = null) { - if ($job instanceof Job) { + if ($job instanceof IJob) { $class = get_class($job); } else { $class = $job; @@ -107,12 +108,12 @@ class JobList implements IJobList { /** * check if a job is in the list * - * @param Job|string $job + * @param IJob|string $job * @param mixed $argument * @return bool */ public function has($job, $argument) { - if ($job instanceof Job) { + if ($job instanceof IJob) { $class = get_class($job); } else { $class = $job; @@ -136,7 +137,7 @@ class JobList implements IJobList { /** * get all jobs in the list * - * @return Job[] + * @return IJob[] */ public function getAll() { $query = $this->connection->getQueryBuilder(); @@ -159,7 +160,7 @@ class JobList implements IJobList { /** * get the next job in the list * - * @return Job + * @return IJob|null */ public function getNext() { $lastId = $this->getLastJob(); @@ -206,7 +207,7 @@ class JobList implements IJobList { /** * @param int $id - * @return Job|null + * @return IJob|null */ public function getById($id) { $query = $this->connection->getQueryBuilder(); @@ -228,7 +229,7 @@ class JobList implements IJobList { * get the job object from a row in the db * * @param array $row - * @return Job + * @return IJob|null */ private function buildJob($row) { $class = $row['class']; @@ -254,7 +255,7 @@ class JobList implements IJobList { /** * set the job that was last ran * - * @param Job $job + * @param IJob $job */ public function setLastJob($job) { $this->config->setAppValue('backgroundjob', 'lastjob', $job->getId()); @@ -263,16 +264,16 @@ class JobList implements IJobList { /** * get the id of the last ran job * - * @return string + * @return int */ public function getLastJob() { - return $this->config->getAppValue('backgroundjob', 'lastjob', 0); + return (int) $this->config->getAppValue('backgroundjob', 'lastjob', 0); } /** * set the lastRun of $job to now * - * @param Job $job + * @param IJob $job */ public function setLastRun($job) { $query = $this->connection->getQueryBuilder(); diff --git a/lib/public/backgroundjob/ijoblist.php b/lib/public/backgroundjob/ijoblist.php index 384f8b3d801..51431c42a67 100644 --- a/lib/public/backgroundjob/ijoblist.php +++ b/lib/public/backgroundjob/ijoblist.php @@ -36,7 +36,6 @@ interface IJobList { * * @param \OCP\BackgroundJob\IJob|string $job * @param mixed $argument The argument to be passed to $job->run() when the job is exectured - * @return void * @since 7.0.0 */ public function add($job, $argument = null); @@ -46,7 +45,6 @@ interface IJobList { * * @param \OCP\BackgroundJob\IJob|string $job * @param mixed $argument - * @return void * @since 7.0.0 */ public function remove($job, $argument = null); @@ -72,14 +70,14 @@ interface IJobList { /** * get the next job in the list * - * @return \OCP\BackgroundJob\IJob + * @return \OCP\BackgroundJob\IJob|null * @since 7.0.0 */ public function getNext(); /** * @param int $id - * @return \OCP\BackgroundJob\IJob + * @return \OCP\BackgroundJob\IJob|null * @since 7.0.0 */ public function getById($id); @@ -88,7 +86,6 @@ interface IJobList { * set the job that was last ran to the current time * * @param \OCP\BackgroundJob\IJob $job - * @return void * @since 7.0.0 */ public function setLastJob($job); @@ -105,7 +102,6 @@ interface IJobList { * set the lastRun of $job to now * * @param \OCP\BackgroundJob\IJob $job - * @return void * @since 7.0.0 */ public function setLastRun($job);