From 4aa98665b8de464c1eb692ba82f2f7ba851c1a6d Mon Sep 17 00:00:00 2001 From: Wilco Louwerse Date: Fri, 13 Dec 2024 15:18:14 +0100 Subject: [PATCH 1/2] Fix for Job test runs & better logging for jobs in general --- lib/Controller/JobsController.php | 6 +- lib/Controller/SynchronizationsController.php | 62 ++++++++-------- lib/Cron/ActionTask.php | 72 ++++++++++++------- lib/Db/JobLogMapper.php | 20 ++++++ 4 files changed, 106 insertions(+), 54 deletions(-) diff --git a/lib/Controller/JobsController.php b/lib/Controller/JobsController.php index 4ffe28b..e5f2ab6 100644 --- a/lib/Controller/JobsController.php +++ b/lib/Controller/JobsController.php @@ -236,13 +236,15 @@ public function run(int $id): JSONResponse } try { - $this->jobList->getById($job->getJobListId())->start($this->jobList); + $this->jobList->getById($job->getJobListId()) + ->setArgument(['forceRun' => true]) + ->start($this->jobList); $lastLog = $this->jobLogMapper->getLastCallLog(); if ($lastLog !== null) { return new JSONResponse(data: $lastLog, statusCode: 200); } - return new JSONResponse(data: ['error' => 'No job log could be found, job did not went succesfully or failed to log anything'], statusCode: 500); + return new JSONResponse(data: ['error' => 'No job log could be found, job did not go successfully or failed to log anything'], statusCode: 500); } catch (Exception $exception) { return new JSONResponse(data: ['error' => $exception->getMessage()], statusCode: 400); } diff --git a/lib/Controller/SynchronizationsController.php b/lib/Controller/SynchronizationsController.php index 9ceb1e9..85b5171 100644 --- a/lib/Controller/SynchronizationsController.php +++ b/lib/Controller/SynchronizationsController.php @@ -2,6 +2,7 @@ namespace OCA\OpenConnector\Controller; +use GuzzleHttp\Exception\GuzzleException; use OCA\OpenConnector\Service\ObjectService; use OCA\OpenConnector\Service\SearchService; use OCA\OpenConnector\Service\SynchronizationService; @@ -15,6 +16,8 @@ use OCP\IRequest; use Exception; use OCP\AppFramework\Db\DoesNotExistException; +use Psr\Container\ContainerExceptionInterface; +use Psr\Container\NotFoundExceptionInterface; class SynchronizationsController extends Controller { @@ -213,33 +216,36 @@ public function logs(int $id): JSONResponse } } - /** - * Tests a synchronization - * - * This method tests a synchronization without persisting anything to the database. - * - * @NoAdminRequired - * @NoCSRFRequired - * - * @param int $id The ID of the synchronization - * - * @return JSONResponse A JSON response containing the test results - * - * @example - * Request: - * empty POST - * - * Response: - * { - * "resultObject": { - * "fullName": "John Doe", - * "userAge": 30, - * "contactEmail": "john@example.com" - * }, - * "isValid": true, - * "validationErrors": [] - * } - */ + /** + * Tests a synchronization + * + * This method tests a synchronization without persisting anything to the database. + * + * @NoAdminRequired + * @NoCSRFRequired + * + * @param int $id The ID of the synchronization + * + * @return JSONResponse A JSON response containing the test results + * @throws GuzzleException + * @throws ContainerExceptionInterface + * @throws NotFoundExceptionInterface + * + * @example + * Request: + * empty POST + * + * Response: + * { + * "resultObject": { + * "fullName": "John Doe", + * "userAge": 30, + * "contactEmail": "john@example.com" + * }, + * "isValid": true, + * "validationErrors": [] + * } + */ public function test(int $id): JSONResponse { try { @@ -263,4 +269,4 @@ public function test(int $id): JSONResponse return new JSONResponse($resultFromTest, 200); } -} \ No newline at end of file +} diff --git a/lib/Cron/ActionTask.php b/lib/Cron/ActionTask.php index 0ad5a34..be57843 100644 --- a/lib/Cron/ActionTask.php +++ b/lib/Cron/ActionTask.php @@ -64,27 +64,52 @@ public function run($argument) { // if we do not have a job id then everything is wrong if (isset($arguments['jobId']) === true && is_int($argument['jobId']) === true) { - return; + return $this->jobLogMapper->createFromArray([ + 'jobId' => 'null', + 'level' => 'ERROR', + 'message' => "Couldn't find a jobId in the action arguments" + ]); } - // Let's get the job, the user might have deleted it in the mean time + // Let's get the job, the user might have deleted it in the meantime try { $job = $this->jobMapper->find($argument['jobId']); } catch (Exception $e) { - return; + return $this->jobLogMapper->createFromArray([ + 'jobId' => $argument['jobId'], + 'level' => 'ERROR', + 'message' => "Couldn't find a Job with this jobId, message: ".$e->getMessage() + ]); } + $forceRun = false; + $stackTrace = []; + if (isset($arguments['forceRun']) === true && $arguments['forceRun'] === true) { + $forceRun = true; + $stackTrace[] = $message = 'Doing a force run for this job, ignoring "enabled" & "nextRun" check...'; + $this->jobLogMapper->createForJob($job, [ + 'level' => 'INFO', + 'message' => $message + ]); + } + // If the job is not enabled, we don't need to do anything - if ($job->getIsEnabled() === false) { - return; + if ($forceRun === false && $job->getIsEnabled() === false) { + return $this->jobLogMapper->createForJob($job, [ + 'level' => 'WARNING', + 'message' => 'This job is disabled' + ]); } - // if the next run is in the the future, we don't need to do anything - if ($job->getNextRun() !== null && $job->getNextRun() > new DateTime()) { - return; + // if the next run is in the future, we don't need to do anything + if ($forceRun === false && $job->getNextRun() !== null && $job->getNextRun() > new DateTime()) { + return $this->jobLogMapper->createForJob($job, [ + 'level' => 'WARNING', + 'message' => 'Next Run is still in the future for this job' + ]); } - if(empty($job->getUserId()) === false && $this->userSession->getUser() === null) { + if (empty($job->getUserId()) === false && $this->userSession->getUser() === null) { $user = $this->userManager->get($job->getUserId()); $this->userSession->setUser($user); } @@ -102,26 +127,23 @@ public function run($argument) $executionTime = ( $time_end - $time_start ) * 1000; // deal with single run - if ($job->isSingleRun() === true) { + if ($forceRun === false && $job->isSingleRun() === true) { $job->setIsEnabled(false); } - // Update the job - $job->setLastRun(new DateTime()); - $nextRun = new DateTime('now + '.$job->getInterval().' seconds'); - $nextRun->setTime(hour: $nextRun->format('H'), minute: $nextRun->format('i')); - $job->setNextRun($nextRun); - $this->jobMapper->update($job); + if ($forceRun === false) { + $job->setLastRun(new DateTime()); + $nextRun = new DateTime('now + '.$job->getInterval().' seconds'); + $nextRun->setTime(hour: $nextRun->format('H'), minute: $nextRun->format('i')); + $job->setNextRun($nextRun); + $this->jobMapper->update($job); + } // Log the job - $jobLog = $this->jobLogMapper->createFromArray([ - 'jobId' => $job->getId(), - 'jobClass' => $job->getJobClass(), - 'jobListId' => $job->getJobListId(), - 'arguments' => $job->getArguments(), - 'lastRun' => $job->getLastRun(), - 'nextRun' => $job->getNextRun(), + $jobLog = $this->jobLogMapper->createForJob($job, [ + 'level' => 'INFO', + 'message' => 'Succes', 'executionTime' => $executionTime ]); @@ -134,10 +156,12 @@ public function run($argument) $jobLog->setMessage($result['message']); } if (isset($result['stackTrace']) === true) { - $jobLog->setStackTrace($result['stackTrace']); + $stackTrace = array_merge($stackTrace, $result['stackTrace']); } } + $jobLog->setStackTrace($stackTrace); + $this->jobLogMapper->update(entity: $jobLog); // Let's report back about what we have just done diff --git a/lib/Db/JobLogMapper.php b/lib/Db/JobLogMapper.php index cc0dc68..cea21ca 100644 --- a/lib/Db/JobLogMapper.php +++ b/lib/Db/JobLogMapper.php @@ -56,8 +56,28 @@ public function findAll(?int $limit = null, ?int $offset = null, ?array $filters return $this->findEntities($qb); } + public function createForJob(Job $job, array $object): JobLog + { + $jobObject = [ + 'jobId' => $job->getId(), + 'jobClass' => $job->getJobClass(), + 'jobListId' => $job->getJobListId(), + 'arguments' => $job->getArguments(), + 'lastRun' => $job->getLastRun(), + 'nextRun' => $job->getNextRun(), + ]; + + $object = array_merge($jobObject, $object); + + return $this->createFromArray($object); + } + public function createFromArray(array $object): JobLog { + if (isset($object['executionTime']) === false) { + $object['executionTime'] = 0; + } + $obj = new JobLog(); $obj->hydrate($object); // Set uuid From 0ffe91d4e9600ea9e3c016661e6a59f49d8c41eb Mon Sep 17 00:00:00 2001 From: Wilco Louwerse Date: Fri, 13 Dec 2024 16:08:44 +0100 Subject: [PATCH 2/2] Fixes --- lib/Controller/JobsController.php | 17 ++++++++++------- lib/Cron/ActionTask.php | 12 ++++-------- lib/Service/JobService.php | 4 ++-- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/lib/Controller/JobsController.php b/lib/Controller/JobsController.php index e5f2ab6..201657a 100644 --- a/lib/Controller/JobsController.php +++ b/lib/Controller/JobsController.php @@ -236,13 +236,16 @@ public function run(int $id): JSONResponse } try { - $this->jobList->getById($job->getJobListId()) - ->setArgument(['forceRun' => true]) - ->start($this->jobList); - $lastLog = $this->jobLogMapper->getLastCallLog(); - if ($lastLog !== null) { - return new JSONResponse(data: $lastLog, statusCode: 200); - } + $job = $this->jobList->getById($job->getJobListId()); + if ($job !== null) { + $job->setArgument(['jobId' => $id, 'forceRun' => true]); + $job->start($this->jobList); + + $lastLog = $this->jobLogMapper->getLastCallLog(); + if ($lastLog !== null && ($lastLog->getJobId() === null || (int) $lastLog->getJobId() === $id)) { + return new JSONResponse(data: $lastLog, statusCode: 200); + } + } return new JSONResponse(data: ['error' => 'No job log could be found, job did not go successfully or failed to log anything'], statusCode: 500); } catch (Exception $exception) { diff --git a/lib/Cron/ActionTask.php b/lib/Cron/ActionTask.php index be57843..ec412b3 100644 --- a/lib/Cron/ActionTask.php +++ b/lib/Cron/ActionTask.php @@ -63,11 +63,11 @@ public function __construct( public function run($argument) { // if we do not have a job id then everything is wrong - if (isset($arguments['jobId']) === true && is_int($argument['jobId']) === true) { + if (isset($argument['jobId']) === false || is_int($argument['jobId']) === false) { return $this->jobLogMapper->createFromArray([ 'jobId' => 'null', 'level' => 'ERROR', - 'message' => "Couldn't find a jobId in the action arguments" + 'message' => "Couldn't find a jobId in the action argument" ]); } @@ -84,13 +84,9 @@ public function run($argument) $forceRun = false; $stackTrace = []; - if (isset($arguments['forceRun']) === true && $arguments['forceRun'] === true) { + if (isset($argument['forceRun']) === true && $argument['forceRun'] === true) { $forceRun = true; - $stackTrace[] = $message = 'Doing a force run for this job, ignoring "enabled" & "nextRun" check...'; - $this->jobLogMapper->createForJob($job, [ - 'level' => 'INFO', - 'message' => $message - ]); + $stackTrace[] = 'Doing a force run for this job, ignoring "enabled" & "nextRun" check...'; } // If the job is not enabled, we don't need to do anything diff --git a/lib/Service/JobService.php b/lib/Service/JobService.php index 8185646..e5da76a 100644 --- a/lib/Service/JobService.php +++ b/lib/Service/JobService.php @@ -51,10 +51,10 @@ public function scheduleJob(Job $job): Job $arguments['jobId'] = $job->getId(); if (!$job->getScheduleAfter()) { - $iJob = $this->jobList->add($this->actionTask::class, $arguments); + $this->jobList->add($this->actionTask::class, $arguments); } else { $runAfter = $job->getScheduleAfter()->getTimestamp(); - $iJob = $this->jobList->scheduleAfter($this->actionTask::class, $runAfter, $arguments); + $this->jobList->scheduleAfter($this->actionTask::class, $runAfter, $arguments); } // Set the job list id