Refactor display only invalid jobs

changelog/unreleased/40846

@@ -0,0 +1,13 @@
+Change: display and delete invalid background jobs
+
+Background jobs can be no longer valid because they are from an old version of
+an app, or from an app that has been disabled. These jobs can now be listed
+with the command:
+
+occ background:queue:status --display-invalid-jobs
+
+And can be deleted with the command:
+
+occ background:queue:delete <Job ID>
+
+https://github.com/owncloud/core/pull/40846

core/Command/Background/Queue/Delete.php

@@ -51,8 +51,7 @@ protected function configure() {
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$id = $input->getArgument('Job ID');
 
-		$job = $this->jobList->getById($id);
-		if ($job === null) {
+		if (!$this->jobList->jobIdExists($id)) {
 			$output->writeln("Job with ID <$id> is not known.");
 			return 1;
 		}

core/Command/Background/Queue/Status.php

@@ -26,6 +26,7 @@
 use Symfony\Component\Console\Command\Command;
 use Symfony\Component\Console\Helper\Table;
 use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Input\InputOption;
 use Symfony\Component\Console\Output\OutputInterface;
 
 class Status extends Command {
@@ -40,7 +41,8 @@ public function __construct(IJobList $jobList) {
 	protected function configure() {
 		$this
 			->setName('background:queue:status')
-			->setDescription('List queue status');
+			->setDescription('List queue status')
+			->addOption('display-only-invalid-jobs', null, InputOption::VALUE_NONE, 'Only display jobs that are no longer valid');
 	}
 
 	private function getJobArgumentAsString($argument) {
@@ -58,17 +60,20 @@ private function getJobArgumentAsString($argument) {
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$t = new Table($output);
 		$t->setHeaders(['Job ID', 'Job', 'Job Arguments', 'Last Run', 'Last Checked', 'Reserved At', 'Execution Duration (s)']);
-		$this->jobList->listJobs(function (IJob $job) use ($t) {
-			$t->addRow([
-				$job->getId(),
-				\get_class($job),
-				$this->getJobArgumentAsString($job->getArgument()),
-				$job->getLastRun() == 0 ? 'N/A' : \date('c', $job->getLastRun()),
-				\date('c', $job->getLastChecked()),
-				$job->getReservedAt() == 0 ? 'N/A' : \date('c', $job->getReservedAt()),
-				$job->getExecutionDuration() == -1 ? 'N/A' : $job->getExecutionDuration(),
-			]);
-		});
+		$this->jobList->listJobs(
+			function (IJob $job, string $classAsString) use ($t) {
+				$t->addRow([
+					$job->getId(),
+					$classAsString,
+					$this->getJobArgumentAsString($job->getArgument()),
+					$job->getLastRun() == 0 ? 'N/A' : \date('c', $job->getLastRun()),
+					\date('c', $job->getLastChecked()),
+					$job->getReservedAt() == 0 ? 'N/A' : \date('c', $job->getReservedAt()),
+					$job->getExecutionDuration() == -1 ? 'N/A' : $job->getExecutionDuration(),
+				]);
+			},
+			$input->getOption('display-only-invalid-jobs')
+		);
 		$t->render();
 		return 0;
 	}

lib/private/BackgroundJob/JobList.php

@@ -24,6 +24,7 @@
 
 namespace OC\BackgroundJob;
 
+use OC\Command\CommandJob;
 use OCP\AppFramework\QueryException;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJob;
@@ -240,14 +241,37 @@ public function getById($id) {
 		}
 	}
 
+	/**
+	 * @param int $id
+	 * @return bool
+	 */
+	public function jobIdExists($id) {
+		$query = $this->connection->getQueryBuilder();
+		$query->select('*')
+			->from('jobs')
+			->where($query->expr()->eq('id', $query->createNamedParameter($id, IQueryBuilder::PARAM_INT)));
+		$result = $query->execute();
+		$row = $result->fetch();
+		$result->closeCursor();
+
+		if ($row) {
+			return true;
+		} else {
+			return false;
+		}
+	}
+
 	/**
 	 * get the job object from a row in the db
 	 *
 	 * @param array $row
+	 * @param bool $includeInvalidJobs
+	 * @param bool $onlyInvalidJobs
 	 * @return IJob|null
 	 */
-	private function buildJob($row) {
+	private function buildJob($row, $includeInvalidJobs = false, $onlyInvalidJobs = false) {
 		try {
+			$jobIsValid = true;
 			try {
 				// Try to load the job as a service
 				/** @var IJob $job */
@@ -258,11 +282,21 @@ private function buildJob($row) {
 					$class = $row['class'];
 					$job = new $class();
 				} else {
-					// job from disabled app or old version of an app, no need to do anything
-					return null;
+					// job is from disabled app or old version of an app
+					$jobIsValid = false;
+					if ($includeInvalidJobs) {
+						// Use CommandJob as a "template" for this job that has an unknown class
+						$job = new CommandJob();
+					} else {
+						return null;
+					}
 				}
 			}
 
+			if ($jobIsValid && $onlyInvalidJobs) {
+				return null;
+			}
+
 			$job->setId($row['id']);
 			$job->setLastRun($row['last_run']);
 			$job->setArgument(\json_decode($row['argument'], true));
@@ -332,18 +366,20 @@ public function setExecutionTime($job, $timeTaken) {
 	}
 
 	/**
+	 * @param \Closure $callback
+	 * @param bool $onlyInvalidJobs
 	 * @inheritdoc
 	 */
-	public function listJobs(\Closure $callback) {
+	public function listJobs(\Closure $callback, bool $onlyInvalidJobs = false) {
 		$query = $this->connection->getQueryBuilder();
 		$query->select('*')
 			->from('jobs');
 		$result = $query->execute();
 
 		while ($row = $result->fetch()) {
-			$job = $this->buildJob($row);
+			$job = $this->buildJob($row, true, $onlyInvalidJobs);
 			if ($job) {
-				if ($callback($job) === false) {
+				if ($callback($job, $row['class']) === false) {
 					break;
 				}
 			}

lib/public/BackgroundJob/IJobList.php

@@ -84,6 +84,13 @@ public function getNext();
 	 */
 	public function getById($id);
 
+	/**
+	 * @param int $id
+	 * @return bool
+	 * @since 10.13.0
+	 */
+	public function jobIdExists($id);
+
 	/**
 	 * set the job that was last ran to the current time
 	 *
@@ -129,12 +136,12 @@ public function setLastRun($job);
 	public function setExecutionTime($job, $timeTaken);
 
 	/**
-	 * iterate over all jobs in the queue
+	 * iterate over jobs in the queue
 	 *
 	 * @return void
 	 * @since 10.2.0
 	 */
-	public function listJobs(\Closure $callback);
+	public function listJobs(\Closure $callback, bool $onlyInvalidJobs = false);
 
 	/**
 	 * remove a specific job by id

tests/Core/Command/Background/Queue/DeleteTest.php

@@ -41,9 +41,9 @@ public function setUp(): void {
 		parent::setUp();
 
 		$this->jobList = $this->createMock(IJobList::class);
-		$this->jobList->expects($this->any())->method('getById')
+		$this->jobList->expects($this->any())->method('jobIdExists')
 			->willReturnCallback(function ($id) {
-				return ($id !== '666') ? true : null;
+				return ($id !== '666') ? true : false;
 			});
 
 		$command = new Delete($this->jobList);

tests/Core/Command/Background/Queue/StatusTest.php

@@ -49,14 +49,15 @@ public function setUp(): void {
 	}
 
 	public function testCommandInput() {
-		$this->jobList->expects($this->any())->method('listJobs')
+		$this->jobList->expects($this->once())->method('listJobs')
+			->with($this->isType('callable'), false)
 			->willReturnCallback(function (\Closure $callBack) {
 				$job = new RegularJob();
 				$job->setId(666);
 				$job->setLastChecked(10);
 				$job->setReservedAt(0);
 				$job->setExecutionDuration(-1);
-				$callBack($job);
+				$callBack($job, \get_class($job));
 			});
 
 		$this->commandTester->execute([]);
@@ -73,15 +74,16 @@ public function testCommandInput() {
 	}
 
 	public function testJobWithArray() {
-		$this->jobList->expects($this->any())->method('listJobs')
+		$this->jobList->expects($this->once())->method('listJobs')
+			->with($this->isType('callable'), false)
 			->willReturnCallback(function (\Closure $callBack) {
 				$job = new RegularJob();
 				$job->setId(666);
 				$job->setArgument(['k'=> 'v','test2']);
 				$job->setLastChecked(10);
 				$job->setReservedAt(0);
 				$job->setExecutionDuration(-1);
-				$callBack($job);
+				$callBack($job, \get_class($job));
 			});
 		$this->commandTester->execute([]);
 		$output = $this->commandTester->getDisplay();
@@ -97,7 +99,8 @@ public function testJobWithArray() {
 	}
 
 	public function testJobWithSchedulingInfo() {
-		$this->jobList->expects($this->any())->method('listJobs')
+		$this->jobList->expects($this->once())->method('listJobs')
+			->with($this->isType('callable'), false)
 			->willReturnCallback(function (\Closure $callBack) {
 				$job = new RegularJob();
 				$job->setId(666);
@@ -106,7 +109,7 @@ public function testJobWithSchedulingInfo() {
 				$job->setLastChecked(10);
 				$job->setReservedAt(10);
 				$job->setExecutionDuration(1);
-				$callBack($job);
+				$callBack($job, \get_class($job));
 			});
 		$this->commandTester->execute([]);
 		$output = $this->commandTester->getDisplay();
@@ -116,6 +119,34 @@ public function testJobWithSchedulingInfo() {
 +--------+------------------------------------+-----------------------+---------------------------+---------------------------+---------------------------+------------------------+
 | 666    | OC\BackgroundJob\Legacy\RegularJob | {"k":"v","0":"test2"} | 1970-01-01T00:00:10+00:00 | 1970-01-01T00:00:10+00:00 | 1970-01-01T00:00:10+00:00 | 1                      |
 +--------+------------------------------------+-----------------------+---------------------------+---------------------------+---------------------------+------------------------+
+EOS;
+
+		$this->assertStringContainsString($expected, $output);
+	}
+
+	public function testListingInvalidJob() {
+		$this->jobList->expects($this->once())->method('listJobs')
+			->with($this->isType('callable'), true)
+			->willReturnCallback(function (\Closure $callBack) {
+				$job = new RegularJob();
+				$job->setId(42);
+				$job->setArgument(['k'=> 'v']);
+				$job->setLastRun(20);
+				$job->setLastChecked(40);
+				$job->setReservedAt(15);
+				$job->setExecutionDuration(7);
+				$callBack($job, \get_class($job));
+			});
+		$this->commandTester->execute(
+			['--display-only-invalid-jobs' => true]
+		);
+		$output = $this->commandTester->getDisplay();
+		$expected = <<<EOS
++--------+------------------------------------+---------------+---------------------------+---------------------------+---------------------------+------------------------+
+| Job ID | Job                                | Job Arguments | Last Run                  | Last Checked              | Reserved At               | Execution Duration (s) |
++--------+------------------------------------+---------------+---------------------------+---------------------------+---------------------------+------------------------+
+| 42     | OC\BackgroundJob\Legacy\RegularJob | {"k":"v"}     | 1970-01-01T00:00:20+00:00 | 1970-01-01T00:00:40+00:00 | 1970-01-01T00:00:15+00:00 | 7                      |
++--------+------------------------------------+---------------+---------------------------+---------------------------+---------------------------+------------------------+
 EOS;
 
 		$this->assertStringContainsString($expected, $output);

tests/lib/BackgroundJob/JobListTest.php

@@ -125,6 +125,22 @@ public function testRemoveDifferentArgument($argument) {
 		$this->assertEquals($existingJobs, $jobs);
 	}
 
+	/**
+	 * @dataProvider argumentProvider
+	 * @param $argument
+	 */
+	public function testJobIdExists($argument) {
+		$this->timeFactory->method('getTime')->willReturn(164419800);
+		$job = new TestJob();
+		$this->instance->add($job, $argument);
+		$jobs = $this->getAllSorted();
+		$addedJob = $jobs[\count($jobs) - 1];
+		$addedJobId = $addedJob->getId();
+		$this->assertTrue($this->instance->jobIdExists($addedJobId));
+		$this->instance->remove($job, $argument);
+		$this->assertFalse($this->instance->jobIdExists($addedJobId));
+	}
+
 	/**
 	 * @dataProvider argumentProvider
 	 * @param $argument