diff --git a/CHANGELOG.md b/CHANGELOG.md index 7373e1e8..745fdbd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +* [PR-325](https://github.com/itk-dev/economics/pull/325) + * Fixed the Leantime sync halting silently on a single bad row. A row that cannot be mapped, or that a handler + rejects, logs `Skipping id : ` and the sync moves on. The catches are deliberately narrow — + a `TypeError` from a null field and a handler's `UnrecoverableMessageHandlingException` are skippable, while a + dead database or an unreachable Leantime still halts the run loudly instead of being logged away as a bad row. + * Made the Leantime result mappers null-safe ahead of + [data-api#18](https://github.com/ITK-Leantime/data-api/pull/18): a deleted user is attributed to + `deleted-user-`, a missing name becomes `(no name) `, and rows with no `ticketId`/`projectId`/`id` + are skipped. The tracker id is part of the name placeholder because names are used as lookup keys elsewhere — + `ProjectBillingService` resolves a client by version name. + * A `deleted-user-` attribution no longer overwrites a worker name an earlier sync already stored. + * Fixed the `/deleted` request sending its timestamp as `deletedAfter` rather than `deleted`, which made every + delete-sync pull the entire unpaginated deletion history. Deletion entries with no id are now skipped and logged, + and a single failing entry no longer drops every deletion after it — with the timestamp now applied, a dropped + entry would never come round again. + * Added `LeantimeApiServiceTest::testUpdateWithNullValues()` and `::testDeletedUserFallbackKeepsStoredWorker()`, + and pinned the `/deleted` request body so the parameter name cannot regress. + ## [3.7.0] - 2026-06-26 * [PR-324](https://github.com/itk-dev/economics/pull/324) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index d1208155..bcacd07b 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -1530,7 +1530,6 @@ parameters: count: 1 path: src/Repository/ProjectVersionBudgetRepository.php - - message: '#^Method App\\Repository\\ServiceAgreementRepository\:\:getFilteredPagination\(\) return type has no value type specified in iterable type Knp\\Component\\Pager\\Pagination\\PaginationInterface\.$#' identifier: missingType.iterableValue @@ -2032,7 +2031,7 @@ parameters: - message: '#^Access to an undefined property object\:\:\$id\.$#' identifier: property.notFound - count: 5 + count: 7 path: src/Service/LeantimeApiService.php - @@ -2053,12 +2052,6 @@ parameters: count: 4 path: src/Service/LeantimeApiService.php - - - message: '#^Access to an undefined property object\:\:\$name\.$#' - identifier: property.notFound - count: 4 - path: src/Service/LeantimeApiService.php - - message: '#^Access to an undefined property object\:\:\$plannedHours\.$#' identifier: property.notFound @@ -2113,12 +2106,6 @@ parameters: count: 1 path: src/Service/LeantimeApiService.php - - - message: '#^Access to an undefined property object\:\:\$username\.$#' - identifier: property.notFound - count: 1 - path: src/Service/LeantimeApiService.php - - message: '#^Access to an undefined property object\:\:\$workDate\.$#' identifier: property.notFound @@ -2932,25 +2919,25 @@ parameters: - message: '#^Call to an undefined method object\:\:findAll\(\)\.$#' identifier: method.notFound - count: 24 + count: 32 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Call to an undefined method object\:\:findOneBy\(\)\.$#' identifier: method.notFound - count: 8 + count: 19 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Call to an undefined method object\:\:flush\(\)\.$#' identifier: method.notFound - count: 3 + count: 5 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Call to an undefined method object\:\:persist\(\)\.$#' identifier: method.notFound - count: 12 + count: 17 path: tests/Integration/Service/LeantimeApiServiceTest.php - @@ -2986,13 +2973,13 @@ parameters: - message: '#^Parameter \#1 \$projectTrackerId of method App\\Entity\\Issue\:\:setProjectTrackerId\(\) expects string, int given\.$#' identifier: argument.type - count: 2 + count: 3 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Parameter \#1 \$projectTrackerId of method App\\Entity\\Project\:\:setProjectTrackerId\(\) expects string\|null, int given\.$#' identifier: argument.type - count: 2 + count: 3 path: tests/Integration/Service/LeantimeApiServiceTest.php - @@ -3004,19 +2991,19 @@ parameters: - message: '#^Parameter \#1 \$projectTrackerIssueId of method App\\Entity\\Worklog\:\:setProjectTrackerIssueId\(\) expects string, int given\.$#' identifier: argument.type - count: 2 + count: 3 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Parameter \#1 \$projectTrackerKey of method App\\Entity\\Issue\:\:setProjectTrackerKey\(\) expects string, int given\.$#' identifier: argument.type - count: 2 + count: 3 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Parameter \#1 \$projectTrackerKey of method App\\Entity\\Project\:\:setProjectTrackerKey\(\) expects string\|null, int given\.$#' identifier: argument.type - count: 2 + count: 3 path: tests/Integration/Service/LeantimeApiServiceTest.php - @@ -3028,31 +3015,31 @@ parameters: - message: '#^Parameter \#2 \$messageBus of class App\\Service\\LeantimeApiService constructor expects Symfony\\Component\\Messenger\\MessageBusInterface, object given\.$#' identifier: argument.type - count: 2 + count: 4 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Parameter \#3 \$dataProviderRepository of class App\\Service\\LeantimeApiService constructor expects App\\Repository\\DataProviderRepository, object given\.$#' identifier: argument.type - count: 2 + count: 4 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Parameter \#4 \$dataProviderId of method App\\Service\\LeantimeApiService\:\:updateAsJob\(\) expects int, int\|null given\.$#' identifier: argument.type - count: 8 + count: 13 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Parameter \#4 \$entityManager of class App\\Service\\LeantimeApiService constructor expects Doctrine\\ORM\\EntityManagerInterface, object given\.$#' identifier: argument.type - count: 2 + count: 4 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Parameter \#5 \$projectRepository of class App\\Service\\LeantimeApiService constructor expects App\\Repository\\ProjectRepository, object given\.$#' identifier: argument.type - count: 2 + count: 4 path: tests/Integration/Service/LeantimeApiServiceTest.php - @@ -3154,7 +3141,7 @@ parameters: - message: '#^Call to an undefined method App\\Service\\DataProviderService\:\:method\(\)\.$#' identifier: method.notFound - count: 1 + count: 2 path: tests/Unit/MessageHandler/EntityRemovedFromDataProviderHandlerTest.php - diff --git a/src/MessageHandler/EntityRemovedFromDataProviderHandler.php b/src/MessageHandler/EntityRemovedFromDataProviderHandler.php index 91da47c1..eec0f86c 100644 --- a/src/MessageHandler/EntityRemovedFromDataProviderHandler.php +++ b/src/MessageHandler/EntityRemovedFromDataProviderHandler.php @@ -6,6 +6,7 @@ use App\Entity\Project; use App\Entity\Version; use App\Entity\Worklog; +use App\Exception\NotFoundException; use App\Exception\NotSupportedException; use App\Message\EntityRemovedFromDataProviderMessage; use App\Service\DataProviderService; @@ -34,7 +35,8 @@ public function __invoke(EntityRemovedFromDataProviderMessage $message): void Worklog::class => $this->dataProviderService->worklogRemovedFromDataProvider($message->dataProviderId, (int) $message->projectTrackerId, $message->deletedDate), default => throw new NotSupportedException('classname not supported'), }; - } catch (\Exception $e) { + } catch (NotFoundException|NotSupportedException|\TypeError $e) { + // Narrow on purpose: see UpsertIssueHandler. Infrastructure failures must propagate. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/LeantimeDeleteHandler.php b/src/MessageHandler/LeantimeDeleteHandler.php index 0f63c5b2..64d56d9f 100644 --- a/src/MessageHandler/LeantimeDeleteHandler.php +++ b/src/MessageHandler/LeantimeDeleteHandler.php @@ -2,6 +2,7 @@ namespace App\MessageHandler; +use App\Exception\NotFoundException; use App\Message\LeantimeDeleteMessage; use App\Service\LeantimeApiService; use Psr\Log\LoggerInterface; @@ -27,7 +28,8 @@ public function __invoke(LeantimeDeleteMessage $message): void $message->asyncJobQueue, $message->deletedAfter, ); - } catch (\Exception $e) { + } catch (NotFoundException|\TypeError $e) { + // Narrow on purpose: see UpsertIssueHandler. Infrastructure failures must propagate. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/LeantimeUpdateHandler.php b/src/MessageHandler/LeantimeUpdateHandler.php index b8cb5a0e..d10bb99d 100644 --- a/src/MessageHandler/LeantimeUpdateHandler.php +++ b/src/MessageHandler/LeantimeUpdateHandler.php @@ -2,6 +2,7 @@ namespace App\MessageHandler; +use App\Exception\NotFoundException; use App\Message\LeantimeUpdateMessage; use App\Service\LeantimeApiService; use Psr\Log\LoggerInterface; @@ -32,7 +33,9 @@ public function __invoke(LeantimeUpdateMessage $message): void $message->modifiedAfter, $message->disableModifiedAtCheck, ); - } catch (\Exception $e) { + } catch (NotFoundException|\TypeError $e) { + // Narrow on purpose: see UpsertIssueHandler. A page that fails because Leantime or the + // database is unavailable must be retried, not dropped along with the pages after it. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertIssueHandler.php b/src/MessageHandler/UpsertIssueHandler.php index 47a4d3f6..79707ed4 100644 --- a/src/MessageHandler/UpsertIssueHandler.php +++ b/src/MessageHandler/UpsertIssueHandler.php @@ -2,6 +2,7 @@ namespace App\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertIssueMessage; use App\Service\DataProviderService; use Psr\Log\LoggerInterface; @@ -22,7 +23,10 @@ public function __invoke(UpsertIssueMessage $message): void try { $this->logger->info('Upserting issue: '.$message->issueData->name); $this->dataProviderService->upsertIssue($message->issueData); - } catch (\Exception $e) { + } catch (NotFoundException|\TypeError $e) { + // Narrow on purpose: only a failure describing this one row is unrecoverable. TypeError + // is here because a null source field mapped onto a non-nullable property raises an + // Error, not an Exception. Anything else propagates, so it is retried, not dropped. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertProjectHandler.php b/src/MessageHandler/UpsertProjectHandler.php index 5e1e0564..7c0d563d 100644 --- a/src/MessageHandler/UpsertProjectHandler.php +++ b/src/MessageHandler/UpsertProjectHandler.php @@ -2,6 +2,7 @@ namespace App\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertProjectMessage; use App\Service\DataProviderService; use Psr\Log\LoggerInterface; @@ -22,7 +23,8 @@ public function __invoke(UpsertProjectMessage $message): void try { $this->logger->info('Upserting project: '.$message->projectData->name); $this->dataProviderService->upsertProject($message->projectData); - } catch (\Exception $e) { + } catch (NotFoundException|\TypeError $e) { + // Narrow on purpose: see UpsertIssueHandler. Infrastructure failures must propagate. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertVersionHandler.php b/src/MessageHandler/UpsertVersionHandler.php index 71a91460..de28a27c 100644 --- a/src/MessageHandler/UpsertVersionHandler.php +++ b/src/MessageHandler/UpsertVersionHandler.php @@ -2,6 +2,7 @@ namespace App\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertVersionMessage; use App\Service\DataProviderService; use Psr\Log\LoggerInterface; @@ -22,7 +23,8 @@ public function __invoke(UpsertVersionMessage $message): void try { $this->logger->info('Upserting version: '.$message->versionData->name); $this->dataProviderService->upsertVersion($message->versionData); - } catch (\Exception $e) { + } catch (NotFoundException|\TypeError $e) { + // Narrow on purpose: see UpsertIssueHandler. Infrastructure failures must propagate. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertWorkerHandler.php b/src/MessageHandler/UpsertWorkerHandler.php index a3b9e247..14ba205b 100644 --- a/src/MessageHandler/UpsertWorkerHandler.php +++ b/src/MessageHandler/UpsertWorkerHandler.php @@ -2,6 +2,7 @@ namespace App\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertWorkerMessage; use App\Service\DataProviderService; use Psr\Log\LoggerInterface; @@ -22,7 +23,8 @@ public function __invoke(UpsertWorkerMessage $message): void try { $this->logger->info('Upserting worker: '.$message->workerData->email); $this->dataProviderService->upsertWorker($message->workerData); - } catch (\Exception $e) { + } catch (NotFoundException|\TypeError $e) { + // Narrow on purpose: see UpsertIssueHandler. Infrastructure failures must propagate. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertWorklogHandler.php b/src/MessageHandler/UpsertWorklogHandler.php index 50312a39..8bbe8388 100644 --- a/src/MessageHandler/UpsertWorklogHandler.php +++ b/src/MessageHandler/UpsertWorklogHandler.php @@ -2,6 +2,7 @@ namespace App\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertWorklogMessage; use App\Service\DataProviderService; use Psr\Log\LoggerInterface; @@ -22,7 +23,8 @@ public function __invoke(UpsertWorklogMessage $message): void try { $this->logger->info('Upserting worklog: '.$message->worklogData->projectTrackerId); $this->dataProviderService->upsertWorklog($message->worklogData); - } catch (\Exception $e) { + } catch (NotFoundException|\TypeError $e) { + // Narrow on purpose: see UpsertIssueHandler. Infrastructure failures must propagate. $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/Model/DataProvider/DataProviderIssueData.php b/src/Model/DataProvider/DataProviderIssueData.php index 51bde7ab..209a8883 100644 --- a/src/Model/DataProvider/DataProviderIssueData.php +++ b/src/Model/DataProvider/DataProviderIssueData.php @@ -12,8 +12,8 @@ public function __construct( public string $projectTrackerProjectId, public string $name, public array $epics, - public float $plannedHours, - public float $remainingHours, + public ?float $plannedHours, + public ?float $remainingHours, public ?string $worker, public IssueStatusEnum $status, public ?\DateTimeInterface $dueDate, diff --git a/src/Model/DataProvider/DataProviderWorklogData.php b/src/Model/DataProvider/DataProviderWorklogData.php index 20cb14dd..ae8d708c 100644 --- a/src/Model/DataProvider/DataProviderWorklogData.php +++ b/src/Model/DataProvider/DataProviderWorklogData.php @@ -12,10 +12,13 @@ public function __construct( public \DateTimeInterface $startedDate, public string $username, public float $hours, - public string $kind, + public ?string $kind, public ?\DateTimeInterface $fetchTime, public ?\DateTimeInterface $sourceModifiedDate, public bool $disableModifiedAtCheck = false, + // True when $username is a stand-in the data provider invented because the source had + // none, so it must not overwrite a real name already recorded for this worklog. + public bool $usernameIsPlaceholder = false, ) { } } diff --git a/src/Service/DataProviderService.php b/src/Service/DataProviderService.php index 899f55c5..163315d4 100644 --- a/src/Service/DataProviderService.php +++ b/src/Service/DataProviderService.php @@ -247,11 +247,17 @@ public function upsertWorklog(DataProviderWorklogData $upsertWorklogData): void $worklog->setWorklogId($upsertWorklogData->projectTrackerId); $worklog->setDescription($upsertWorklogData->description); - $worklog->setWorker($upsertWorklogData->username); + + // A stand-in username describes a user the data provider could no longer resolve. It is + // better than losing the worklog, but not better than the name already on record. + if (!$upsertWorklogData->usernameIsPlaceholder || null === $worklog->getWorker()) { + $worklog->setWorker($upsertWorklogData->username); + } + $worklog->setStarted($upsertWorklogData->startedDate); $worklog->setProjectTrackerIssueId($upsertWorklogData->projectTrackerIssueId); $worklog->setTimeSpentSeconds($upsertWorklogData->hours * $this::SECONDS_IN_HOUR); - $worklog->setKind(BillableKindsEnum::tryFrom($upsertWorklogData->kind)); + $worklog->setKind(null !== $upsertWorklogData->kind ? BillableKindsEnum::tryFrom($upsertWorklogData->kind) : null); $worklog->setProject($issue->getProject()); $worklog->setIssue($issue); $worklog->setFetchDate($upsertWorklogData->fetchTime); diff --git a/src/Service/LeantimeApiService.php b/src/Service/LeantimeApiService.php index 1aff412b..33b080ba 100644 --- a/src/Service/LeantimeApiService.php +++ b/src/Service/LeantimeApiService.php @@ -29,6 +29,8 @@ use App\Repository\ProjectRepository; use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; +use Symfony\Component\Messenger\Exception\HandlerFailedException; +use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Component\Messenger\Stamp\TransportNamesStamp; use Symfony\Contracts\HttpClient\HttpClientInterface; @@ -43,6 +45,11 @@ class LeantimeApiService implements DataProviderInterface public const TIMESHEETS = 'timesheets'; public const WORKERS = 'workers'; private const LIMIT = 100; + // Placeholder for a null name; the name columns are not nullable, and dropping the row would + // lose real data — for issues it would make their worklogs unstorable. The tracker id is + // appended so two unnamed entities stay distinguishable: names are used as lookup keys + // elsewhere, e.g. ProjectBillingService resolves a client by version name. + private const NAME_MISSING = '(no name)'; private const QUEUE_ASYNC = 'async'; private const QUEUE_SYNC = 'sync'; @@ -116,7 +123,9 @@ public function deleteAsJob(int $dataProviderId, bool $asyncJobQueue = false, ?\ $params = [ 'types' => $types, - 'deletedAfter' => $deletedAfter?->getTimestamp(), + // The plugin reads 'deleted'; anything else is discarded and the whole deletion + // history is returned, which /deleted does not paginate. + 'deleted' => $deletedAfter?->getTimestamp(), ]; // Get data from Leantime. @@ -137,13 +146,32 @@ public function deleteAsJob(int $dataProviderId, bool $asyncJobQueue = false, ?\ }; foreach ($results->{$type} as $result) { + // Nothing identifies the entity to remove. + if (null === $result->id) { + $this->logger->error(sprintf('Skipping deleted %s entry with no id', $type)); + + continue; + } + $projectTrackerId = $result->id; - $deletedDate = $this->getLeanDateTime($result->deletedDate); - $this->messageBus->dispatch( - new EntityRemovedFromDataProviderMessage($classname, $dataProviderId, $projectTrackerId, $deletedDate), - [new TransportNamesStamp($asyncJobQueue ? $this::QUEUE_ASYNC : $this::QUEUE_SYNC)], - ); + // Now that the request actually filters by timestamp, an entry dropped here is + // dropped for good; before, every run re-read the full history and healed itself. + // So one bad entry must not take the rest of the run with it. + try { + $deletedDate = $this->getLeanDateTime($result->deletedDate); + + $this->messageBus->dispatch( + new EntityRemovedFromDataProviderMessage($classname, $dataProviderId, $projectTrackerId, $deletedDate), + [new TransportNamesStamp($asyncJobQueue ? $this::QUEUE_ASYNC : $this::QUEUE_SYNC)], + ); + } catch (HandlerFailedException|\DateMalformedStringException|\TypeError $e) { + if ($e instanceof HandlerFailedException) { + $this->rethrowUnlessRowLevel($e); + } + + $this->logger->error(sprintf('Skipping deleted %s id %s: %s', $type, $projectTrackerId, $e->getMessage())); + } } } } @@ -187,7 +215,11 @@ public function updateAsJob(string $className, int $startId, int $limit, int $da // Queue upsert. foreach ($data->results as $result) { $this->dispatchUpsertMessage($className, $result, $dataProviderId, $fetchDate, $asyncJobQueue, $dataProviderUrl, $disableModifiedAtCheck); - $startId = $result->id; + + // Pagination walks forward by id; a null would rewind the next page to the start. + if (null !== $result->id) { + $startId = $result->id; + } } $startId = $startId + 1; @@ -208,6 +240,9 @@ public function updateAsJob(string $className, int $startId, int $limit, int $da private function dispatchUpsertMessage(string $className, object $data, int $dataProviderId, \DateTimeInterface $fetchDate, bool $asyncJobQueue = false, ?string $dataProviderUrl = null, bool $disableModifiedAtCheck = false): void { + // A TypeError, not an Exception, is what a null source field mapped onto a non-nullable + // constructor argument raises. Uncaught it escapes the row loop in updateAsJob() before the + // next page is queued, which is how one bad row used to halt the whole sync silently. try { $message = match ($className) { Project::class => new UpsertProjectMessage($this->getProjectUpsertFromResult($data, $dataProviderId, $fetchDate, $dataProviderUrl, $disableModifiedAtCheck)), @@ -217,17 +252,49 @@ private function dispatchUpsertMessage(string $className, object $data, int $dat Worker::class => new UpsertWorkerMessage($this->getWorkerUpsertFromResult($data, $dataProviderId, $fetchDate)), default => null, }; - } catch (\Exception $e) { - $this->logger->error($e->getMessage()); + } catch (NotAcceptableException|\TypeError $e) { + $this->logger->error(sprintf('Skipping %s id %s: %s', $className, $data->id ?? '?', $e->getMessage())); return; } - if (null !== $message) { + if (null === $message) { + return; + } + + // The dispatch needs guarding too: on the sync transport the handler runs inline here, so a + // row the handler rejects surfaces as part of this call. Only that case is skippable — + // rethrowUnlessRowLevel() keeps a dead database or an unreachable Leantime loud. + try { $this->messageBus->dispatch( $message, [new TransportNamesStamp($asyncJobQueue ? $this::QUEUE_ASYNC : $this::QUEUE_SYNC)], ); + } catch (HandlerFailedException $e) { + $this->rethrowUnlessRowLevel($e); + + $this->logger->error(sprintf('Skipping %s id %s: %s', $className, $data->id ?? '?', $e->getMessage())); + } + } + + /** + * Rethrow a handler failure unless every wrapped cause describes a single unusable row. + * + * The handlers mark a row they cannot process as unrecoverable and let everything else through, + * so an exception that is not unrecoverable means the failure was not about this row. + */ + private function rethrowUnlessRowLevel(HandlerFailedException $exception): void + { + $causes = $exception->getWrappedExceptions(recursive: true); + + if ([] === $causes) { + throw $exception; + } + + foreach ($causes as $cause) { + if (!$cause instanceof UnrecoverableMessageHandlingException) { + throw $exception; + } } } @@ -237,7 +304,7 @@ private function getProjectUpsertFromResult(object $result, int $dataProviderId, return new DataProviderProjectData( $dataProviderId, - $result->name, + $result->name ?? $this->missingName($projectTrackerId), $projectTrackerId, $this->linkToProject($projectTrackerId, $dataProviderUrl), $fetchDate, @@ -248,9 +315,14 @@ private function getProjectUpsertFromResult(object $result, int $dataProviderId, private function getVersionUpsertFromResult(object $result, int $dataProviderId, \DateTimeInterface $fetchDate, bool $disableModifiedAtCheck = false): DataProviderVersionData { + // A version cannot exist without a project; Version::$project is not nullable. + if (null === $result->projectId) { + throw new NotAcceptableException('Version upsert not acceptable: projectId is null'); + } + return new DataProviderVersionData( $dataProviderId, - $result->name, + $result->name ?? $this->missingName((string) $result->id), (string) $result->id, (string) $result->projectId, $fetchDate, @@ -261,13 +333,20 @@ private function getVersionUpsertFromResult(object $result, int $dataProviderId, private function getIssueUpsertFromResult(object $result, int $dataProviderId, \DateTimeInterface $fetchDate, ?string $dataProviderUrl = null, bool $disableModifiedAtCheck = false): DataProviderIssueData { + // An issue cannot exist without a project; casting a null projectId to '' would look up no + // project and silently clear the association an existing issue already has, taking its + // worklogs' project with it. + if (null === $result->projectId) { + throw new NotAcceptableException('Issue upsert not acceptable: projectId is null'); + } + $projectTrackerId = (string) $result->id; return new DataProviderIssueData( $projectTrackerId, $dataProviderId, (string) $result->projectId, - $result->name, + $result->name ?? $this->missingName($projectTrackerId), $result->tags, $result->plannedHours, $result->remainingHours, @@ -291,30 +370,57 @@ private function getWorklogUpsertFromResult(object $result, int $dataProviderId, throw new NotAcceptableException('Worklog upsert not acceptable: startedDate is null'); } + // A worklog cannot exist without an issue; Worklog::$issue is not nullable. + if (null === $result->ticketId) { + throw new NotAcceptableException('Worklog upsert not acceptable: ticketId is null'); + } + + // The id is the key the worklog is stored and looked up under. + if (null === $result->id) { + throw new NotAcceptableException('Worklog upsert not acceptable: id is null'); + } + + // A null username means the join found no user row, as Leantime never stores a null one. + // The hours are still real, so keep the worklog and name the departed user. + $username = $result->username ?? null; + $usernameIsPlaceholder = null === $username; + $username ??= 'deleted-user-'.($result->userId ?? 'unknown'); + return new DataProviderWorklogData( $result->id, $dataProviderId, (string) $result->ticketId, $result->description, $startedDate, - $result->username, + $username, $result->hours, $result->kind, $fetchDate, $this->getLeanDateTime($result->modified), $disableModifiedAtCheck, + $usernameIsPlaceholder, ); } private function getWorkerUpsertFromResult(object $result, int $dataProviderId, \DateTimeInterface $fetchDate): DataProviderWorkerData { + // The email is the key workers are matched on, and the name column is not nullable. + if (null === $result->email) { + throw new NotAcceptableException('Worker upsert not acceptable: email is null'); + } + return new DataProviderWorkerData( $result->id, - $result->name, + $result->name ?? $this->missingName((string) $result->id), $result->email, ); } + private function missingName(string $projectTrackerId): string + { + return sprintf('%s %s', self::NAME_MISSING, $projectTrackerId); + } + private function fetchFromLeantime(DataProvider $dataProvider, string $type, array $params): object { $response = $this->post($dataProvider, $type, $params); @@ -343,7 +449,7 @@ private function getLeanDateTime(?string $dateString): ?\DateTimeInterface return new \DateTime($dateString, new \DateTimeZone('UTC')); } - private function convertStatusToEnum(string $statusString): IssueStatusEnum + private function convertStatusToEnum(?string $statusString): IssueStatusEnum { return match ($statusString) { 'NEW' => IssueStatusEnum::NEW, diff --git a/tests/Integration/Service/LeantimeApiServiceTest.php b/tests/Integration/Service/LeantimeApiServiceTest.php index 6215c3ee..df6ed0a2 100644 --- a/tests/Integration/Service/LeantimeApiServiceTest.php +++ b/tests/Integration/Service/LeantimeApiServiceTest.php @@ -159,6 +159,208 @@ public function testUpdate(): void $this->assertEquals((new \DateTime('2025-10-03T13:47:30.000000Z'))->getTimestamp(), $worklog->getSourceModifiedDate()->getTimestamp()); } + /** + * Nullable fields from data-api#18 must not halt the sync. + * + * A row that cannot be mapped is logged and skipped; every other row still imports. + */ + public function testUpdateWithNullValues(): void + { + self::bootKernel(); + $container = self::getContainer(); + + $messageBus = $container->get(MessageBusInterface::class); + $dataProviderRepository = $container->get(DataProviderRepository::class); + $projectRepository = $container->get(ProjectRepository::class); + $versionRepository = $container->get(VersionRepository::class); + $issueRepository = $container->get(IssueRepository::class); + $worklogRepository = $container->get(WorklogRepository::class); + $entityManager = $container->get(EntityManagerInterface::class); + + // Collect the skip messages instead of asserting call counts, so the log stays readable + // when a new skip is added. + $loggedErrors = []; + $loggerMock = $this->createMock(LoggerInterface::class); + $loggerMock->method('error')->willReturnCallback( + function (string $message, array $context = []) use (&$loggedErrors) { + $loggedErrors[] = $message; + } + ); + + $httpClientMock = $this->createMock(HttpClientInterface::class); + $responses = []; + + foreach ([$this->getNullValueProjects(), $this->getNullValueMilestones(), $this->getNullValueTickets(), $this->getNullValueTimesheets()] as $payload) { + $responseMock = $this->createMock(ResponseInterface::class); + $responseMock->method('getStatusCode')->willReturn(200); + $responseMock->method('getContent')->willReturn(json_encode($payload)); + $responses[] = $responseMock; + } + + $httpClientMock->method('request')->willReturn(...$responses); + + $service = new LeantimeApiService( + $httpClientMock, + $messageBus, + $dataProviderRepository, + $entityManager, + $projectRepository, + $loggerMock, + ); + + $dataProvider = new DataProvider(); + $dataProvider->setName('Data Provider 5 - null values'); + $dataProvider->setEnabled(true); + $dataProvider->setClass(LeantimeApiService::class); + $dataProvider->setUrl('http://localhost/'); + $dataProvider->setSecret('Not so secret'); + $entityManager->persist($dataProvider); + $entityManager->flush(); + + // A project with no name is kept under a placeholder; dropping it would orphan its issues. + // The tracker id is part of the placeholder so two unnamed entities stay distinguishable. + $before = count($projectRepository->findAll()); + $service->updateAsJob(Project::class, 0, 100, $dataProvider->getId()); + $this->assertEquals($before + 2, count($projectRepository->findAll())); + $this->assertEquals('(no name) 70', $projectRepository->findOneBy(['projectTrackerId' => 70, 'dataProvider' => $dataProvider])->getName()); + + // The nameless milestone is kept, the one without a project is skipped: Version::$project + // is not nullable. + $before = count($versionRepository->findAll()); + $service->updateAsJob(Version::class, 0, 100, $dataProvider->getId()); + $this->assertEquals($before + 1, count($versionRepository->findAll())); + $this->assertEquals('(no name) 20', $versionRepository->findOneBy(['projectTrackerId' => 20, 'dataProvider' => $dataProvider])->getName()); + $this->assertNull($versionRepository->findOneBy(['projectTrackerId' => 21, 'dataProvider' => $dataProvider])); + + // A null status maps to OTHER, and null hours are stored as null rather than raising. The + // ticket without a project is skipped rather than stored against no project at all, which + // would also clear the association on any issue that already had one. + $before = count($issueRepository->findAll()); + $service->updateAsJob(Issue::class, 0, 100, $dataProvider->getId()); + $this->assertEquals($before + 2, count($issueRepository->findAll())); + $issue = $issueRepository->findOneBy(['projectTrackerId' => 30, 'dataProvider' => $dataProvider]); + $this->assertEquals('(no name) 30', $issue->getName()); + $this->assertEquals(IssueStatusEnum::OTHER, $issue->getStatus()); + $this->assertNull($issue->getPlanHours()); + $this->assertNull($issueRepository->findOneBy(['projectTrackerId' => 32, 'dataProvider' => $dataProvider])); + + // Hours worked by a deleted user are real billable data, so the worklog is kept and + // attributed via the userId that data-api#18 added. The one with no ticket cannot be + // stored at all, as Worklog::$issue is not nullable. + $before = count($worklogRepository->findAll()); + $service->updateAsJob(Worklog::class, 0, 100, $dataProvider->getId()); + $this->assertEquals($before + 2, count($worklogRepository->findAll())); + + $deletedUserWorklog = $worklogRepository->findOneBy(['worklogId' => 100, 'dataProvider' => $dataProvider]); + $this->assertEquals('deleted-user-42', $deletedUserWorklog->getWorker()); + $this->assertNull($deletedUserWorklog->getKind()); + + $this->assertNull($worklogRepository->findOneBy(['worklogId' => 101, 'dataProvider' => $dataProvider])); + + $unknownUserWorklog = $worklogRepository->findOneBy(['worklogId' => 102, 'dataProvider' => $dataProvider]); + $this->assertEquals('deleted-user-unknown', $unknownUserWorklog->getWorker()); + + // Rows 103 and 104 are the regression probes for the two ways a single row used to stop + // the sync: a TypeError, which catch (\Exception) could not see, and a handler failure, + // which happened outside the try because the dispatch sat there. + $this->assertNull($worklogRepository->findOneBy(['worklogId' => 103, 'dataProvider' => $dataProvider])); + $this->assertNull($worklogRepository->findOneBy(['worklogId' => 104, 'dataProvider' => $dataProvider])); + + // Each skipped row is reported once, so a halt could never be silent. + $this->assertCount(5, $loggedErrors); + $this->assertStringContainsString('Version upsert not acceptable: projectId is null', $loggedErrors[0]); + $this->assertStringContainsString('Issue upsert not acceptable: projectId is null', $loggedErrors[1]); + $this->assertStringContainsString('ticketId is null', $loggedErrors[2]); + $this->assertStringContainsString('Skipping App\Entity\Worklog id 103', $loggedErrors[3]); + $this->assertStringContainsString('999', $loggedErrors[4]); + } + + /** + * A stand-in username must not replace a worker name already on record. + * + * Keeping the worklog is worth a placeholder; overwriting a name an earlier sync stored is not. + */ + public function testDeletedUserFallbackKeepsStoredWorker(): void + { + self::bootKernel(); + $container = self::getContainer(); + + $messageBus = $container->get(MessageBusInterface::class); + $dataProviderRepository = $container->get(DataProviderRepository::class); + $projectRepository = $container->get(ProjectRepository::class); + $worklogRepository = $container->get(WorklogRepository::class); + $entityManager = $container->get(EntityManagerInterface::class); + + $loggerMock = $this->createMock(LoggerInterface::class); + + $responseMock = $this->createMock(ResponseInterface::class); + $responseMock->method('getStatusCode')->willReturn(200); + $responseMock->method('getContent')->willReturn(json_encode($this->getDeletedUserTimesheets())); + + $httpClientMock = $this->createMock(HttpClientInterface::class); + $httpClientMock->method('request')->willReturn($responseMock); + + $service = new LeantimeApiService( + $httpClientMock, + $messageBus, + $dataProviderRepository, + $entityManager, + $projectRepository, + $loggerMock, + ); + + $dataProvider = new DataProvider(); + $dataProvider->setName('Data Provider 6 - deleted user'); + $dataProvider->setEnabled(true); + $dataProvider->setClass(LeantimeApiService::class); + $dataProvider->setUrl('http://localhost/'); + $dataProvider->setSecret('Not so secret'); + $entityManager->persist($dataProvider); + + $project = new Project(); + $project->setDataProvider($dataProvider); + $project->setProjectTrackerId(80); + $project->setProjectTrackerKey(80); + $project->setName('Project for a departed user'); + $project->setProjectTrackerProjectUrl('http://localhost/'); + $project->setInclude(true); + $project->setIsBillable(true); + $entityManager->persist($project); + + $issue = new Issue(); + $issue->setDataProvider($dataProvider); + $issue->setProject($project); + $issue->setProjectTrackerId(40); + $issue->setProjectTrackerKey(40); + $issue->setName('Issue for a departed user'); + $issue->setStatus(IssueStatusEnum::DONE); + $issue->setLinkToIssue('www.example.com'); + $entityManager->persist($issue); + + $worklog = new Worklog(); + $worklog->setDataProvider($dataProvider); + $worklog->setProject($project); + $worklog->setIssue($issue); + $worklog->setProjectTrackerIssueId(40); + $worklog->setWorklogId(200); + $worklog->setDescription('Recorded while the user still existed'); + $worklog->setIsBilled(false); + $worklog->setWorker('real.person@example.com'); + $worklog->setTimeSpentSeconds(60 * 60); + $worklog->setStarted(new \DateTime('2026-01-04T22:00:00.000000Z')); + $worklog->setKind(BillableKindsEnum::GENERAL_BILLABLE); + $entityManager->persist($worklog); + $entityManager->flush(); + + $service->updateAsJob(Worklog::class, 0, 100, $dataProvider->getId()); + + $stored = $worklogRepository->findOneBy(['worklogId' => 200, 'dataProvider' => $dataProvider]); + + // The rest of the row was upserted, so the sync did run over it — only the worker was left. + $this->assertEquals(60 * 60 * 3, $stored->getTimeSpentSeconds()); + $this->assertEquals('real.person@example.com', $stored->getWorker()); + } + public function testDeleted(): void { self::bootKernel(); @@ -180,7 +382,15 @@ public function testDeleted(): void $responseMock->method('getStatusCode')->willReturn(200); $responseMock->method('getContent')->willReturn(json_encode($this->getDeletedData())); - $httpClientMock->method('request')->willReturn($responseMock); + // Capture what was actually sent, so the request body can be asserted after the call. + $requestJson = null; + $httpClientMock->expects($this->once()) + ->method('request') + ->willReturnCallback(function (string $method, string $url, array $options) use ($responseMock, &$requestJson): ResponseInterface { + $requestJson = $options['json'] ?? null; + + return $responseMock; + }); $service = new LeantimeApiService( $httpClientMock, @@ -347,13 +557,27 @@ public function testDeleted(): void $entityManager->clear(); - $service->deleteAsJob($id, false, new \DateTime('2025-10-06T11:36:08.000000Z')); + $deletedAfter = new \DateTime('2025-10-06T11:36:08.000000Z'); + + $service->deleteAsJob($id, false, $deletedAfter); + + // The plugin only reads 'deleted'. Under any other key the timestamp is silently discarded + // and every run pulls the entire deletion history, which /deleted does not paginate. + $this->assertSame( + [ + 'types' => ['timesheets', 'tickets', 'milestones', 'projects'], + 'deleted' => $deletedAfter->getTimestamp(), + ], + $requestJson + ); $countProjectsAfterDelete = count($projectRepository->findAll()); $countVersionsAfterDelete = count($versionRepository->findAll()); $countIssuesAfterDelete = count($issueRepository->findAll()); $countWorklogsAfterDelete = count($worklogRepository->findAll()); + // The two worklogs sit behind an entry with an unparsable date. If that entry escaped the + // loop instead of being skipped, neither would ever be reached. $this->assertEquals($countWorklogsBeforeCreate + 1, $countWorklogsAfterDelete); $this->assertEquals($countIssuesBeforeCreate + 1, $countIssuesAfterDelete); // Versions can always be removed. @@ -370,6 +594,217 @@ public function testDeleted(): void $this->assertEquals(new \DateTime('2025-10-24T11:36:08.000000Z'), $worklog1->getSourceDeletedDate()); } + private function getNullValueProjects(): object + { + return json_decode(' + { + "parameters": { + "start": 0, + "limit": 100 + }, + "resultsCount": 2, + "results": [ + { + "id": 70, + "name": null, + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 71, + "name": "Project with a name", + "modified": "2026-01-05T09:00:00.000000Z" + } + ] + } + ', null, 512, JSON_THROW_ON_ERROR); + } + + private function getNullValueMilestones(): object + { + return json_decode(' + { + "parameters": { + "start": 0, + "limit": 100 + }, + "resultsCount": 2, + "results": [ + { + "id": 20, + "projectId": 70, + "name": null, + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 21, + "projectId": null, + "name": "Milestone without a project", + "modified": "2026-01-05T09:00:00.000000Z" + } + ] + } + ', null, 512, JSON_THROW_ON_ERROR); + } + + private function getNullValueTickets(): object + { + return json_decode(' + { + "parameters": { + "start": 0, + "limit": 100 + }, + "resultsCount": 3, + "results": [ + { + "id": 30, + "projectId": 70, + "name": null, + "status": null, + "milestoneId": null, + "tags": [], + "worker": null, + "plannedHours": null, + "remainingHours": null, + "dueDate": null, + "resolutionDate": null, + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 31, + "projectId": 70, + "name": "Ticket with a name", + "status": "DONE", + "milestoneId": null, + "tags": [], + "worker": "admin@example.com", + "plannedHours": 4, + "remainingHours": 2, + "dueDate": null, + "resolutionDate": null, + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 32, + "projectId": null, + "name": "Ticket without a project", + "status": "NEW", + "milestoneId": null, + "tags": [], + "worker": null, + "plannedHours": null, + "remainingHours": null, + "dueDate": null, + "resolutionDate": null, + "modified": "2026-01-05T09:00:00.000000Z" + } + ] + } + ', null, 512, JSON_THROW_ON_ERROR); + } + + private function getNullValueTimesheets(): object + { + return json_decode(' + { + "parameters": { + "start": 0, + "limit": 100 + }, + "resultsCount": 5, + "results": [ + { + "id": 100, + "ticketId": 30, + "projectId": 70, + "description": "Hours worked by a since deleted user", + "hours": 2.5, + "userId": 42, + "username": null, + "kind": null, + "workDate": "2026-01-04T22:00:00.000000Z", + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 101, + "ticketId": null, + "projectId": null, + "description": "Timesheet with no ticket", + "hours": 1, + "userId": 1, + "username": "admin@example.com", + "kind": "GENERAL_BILLABLE", + "workDate": "2026-01-04T22:00:00.000000Z", + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 102, + "ticketId": 31, + "projectId": 70, + "description": "Deleted user without a userId", + "hours": 3, + "userId": null, + "username": null, + "kind": "TESTING", + "workDate": "2026-01-04T22:00:00.000000Z", + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 103, + "ticketId": 31, + "projectId": 70, + "description": "Null in a field the API declares non-nullable", + "hours": null, + "userId": 1, + "username": "admin@example.com", + "kind": "GENERAL_BILLABLE", + "workDate": "2026-01-04T22:00:00.000000Z", + "modified": "2026-01-05T09:00:00.000000Z" + }, + { + "id": 104, + "ticketId": 999, + "projectId": 70, + "description": "References a ticket that was never synced", + "hours": 1, + "userId": 1, + "username": "admin@example.com", + "kind": "GENERAL_BILLABLE", + "workDate": "2026-01-04T22:00:00.000000Z", + "modified": "2026-01-05T09:00:00.000000Z" + } + ] + } + ', null, 512, JSON_THROW_ON_ERROR); + } + + private function getDeletedUserTimesheets(): object + { + return json_decode(' + { + "parameters": { + "start": 0, + "limit": 100 + }, + "resultsCount": 1, + "results": [ + { + "id": 200, + "ticketId": 40, + "projectId": 80, + "description": "Recorded while the user still existed", + "hours": 3, + "userId": 42, + "username": null, + "kind": "GENERAL_BILLABLE", + "workDate": "2026-01-04T22:00:00.000000Z", + "modified": "2026-01-06T09:00:00.000000Z" + } + ] + } + ', null, 512, JSON_THROW_ON_ERROR); + } + private function getDeletedData(): object { return json_decode(' @@ -415,6 +850,14 @@ private function getDeletedData(): object } ], "timesheets": [ + { + "id": null, + "deletedDate": "2025-10-24T11:36:08.000000Z" + }, + { + "id": 66939, + "deletedDate": "not a date" + }, { "id": 66937, "deletedDate": "2025-10-24T11:36:08.000000Z" @@ -540,6 +983,7 @@ private function getTimesheets($modifiedYear = 2024): object "projectId": 50, "description": "Fisk", "hours": 5.5, + "userId": 1, "kind": "GENERAL_BILLABLE", "username": "admin@example.com", "workDate": "2024-09-23T22:00:00.000000Z", @@ -551,6 +995,7 @@ private function getTimesheets($modifiedYear = 2024): object "projectId": 51, "description": "add", "hours": 1, + "userId": 1, "kind": "GENERAL_BILLABLE", "username": "admin@example.com", "workDate": "2024-09-24T22:00:00.000000Z", diff --git a/tests/Unit/MessageHandler/EntityRemovedFromDataProviderHandlerTest.php b/tests/Unit/MessageHandler/EntityRemovedFromDataProviderHandlerTest.php index 165f8af7..dc1a1915 100644 --- a/tests/Unit/MessageHandler/EntityRemovedFromDataProviderHandlerTest.php +++ b/tests/Unit/MessageHandler/EntityRemovedFromDataProviderHandlerTest.php @@ -6,6 +6,7 @@ use App\Entity\Project; use App\Entity\Version; use App\Entity\Worklog; +use App\Exception\NotFoundException; use App\Message\EntityRemovedFromDataProviderMessage; use App\MessageHandler\EntityRemovedFromDataProviderHandler; use App\Service\DataProviderService; @@ -73,15 +74,33 @@ public function testWorklogClassCallsWorklogRemoved(): void ($this->handler)($message); } - public function testOnExceptionThrowsUnrecoverable(): void + public function testRowLevelFailureThrowsUnrecoverable(): void { $message = new EntityRemovedFromDataProviderMessage(Project::class, 1, 'PT-1', null); $this->service->method('projectRemovedFromDataProvider') - ->willThrowException(new \RuntimeException('fail')); + ->willThrowException(new NotFoundException('fail')); $this->expectException(UnrecoverableMessageHandlingException::class); ($this->handler)($message); } + + public function testInfrastructureFailurePropagates(): void + { + $message = new EntityRemovedFromDataProviderMessage(Project::class, 1, 'PT-1', null); + + $this->service->method('projectRemovedFromDataProvider') + ->willThrowException(new \RuntimeException('the database went away')); + + // Marking this unrecoverable would drop the message and let the caller record it as a + // skipped row, so a broken run would still report success. + try { + ($this->handler)($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } } diff --git a/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php b/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php index ee942adc..b3168698 100644 --- a/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php +++ b/tests/Unit/MessageHandler/LeantimeDeleteHandlerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Unit\MessageHandler; +use App\Exception\NotFoundException; use App\Message\LeantimeDeleteMessage; use App\MessageHandler\LeantimeDeleteHandler; use App\Service\LeantimeApiService; @@ -25,16 +26,35 @@ public function testInvokeCallsDeleteAsJob(): void $handler($message); } - public function testInvokeOnExceptionThrowsUnrecoverable(): void + public function testInvokeOnRowLevelFailureThrowsUnrecoverable(): void { $message = new LeantimeDeleteMessage(1, false, null); $service = $this->createMock(LeantimeApiService::class); - $service->method('deleteAsJob')->willThrowException(new \RuntimeException('fail')); + $service->method('deleteAsJob')->willThrowException(new NotFoundException('fail')); $handler = new LeantimeDeleteHandler($this->createMock(LoggerInterface::class), $service); $this->expectException(UnrecoverableMessageHandlingException::class); $handler($message); } + + public function testInvokeOnInfrastructureFailurePropagates(): void + { + $message = new LeantimeDeleteMessage(1, false, null); + + $service = $this->createMock(LeantimeApiService::class); + $service->method('deleteAsJob')->willThrowException(new \RuntimeException('the database went away')); + + $handler = new LeantimeDeleteHandler($this->createMock(LoggerInterface::class), $service); + + // Marking this unrecoverable would drop the message, so a broken run would report success. + try { + $handler($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } } diff --git a/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php b/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php index 5213d40d..c6490f32 100644 --- a/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php +++ b/tests/Unit/MessageHandler/LeantimeUpdateHandlerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Unit\MessageHandler; +use App\Exception\NotFoundException; use App\Message\LeantimeUpdateMessage; use App\MessageHandler\LeantimeUpdateHandler; use App\Service\LeantimeApiService; @@ -27,16 +28,35 @@ public function testInvokeCallsUpdateAsJob(): void $handler($message); } - public function testInvokeOnExceptionThrowsUnrecoverable(): void + public function testInvokeOnRowLevelFailureThrowsUnrecoverable(): void { $message = new LeantimeUpdateMessage('App\Entity\Project', 0, 100, 1, false, null); $service = $this->createMock(LeantimeApiService::class); - $service->method('updateAsJob')->willThrowException(new \RuntimeException('fail')); + $service->method('updateAsJob')->willThrowException(new NotFoundException('fail')); $handler = new LeantimeUpdateHandler($this->createMock(LoggerInterface::class), $service); $this->expectException(UnrecoverableMessageHandlingException::class); $handler($message); } + + public function testInvokeOnInfrastructureFailurePropagates(): void + { + $message = new LeantimeUpdateMessage('App\Entity\Project', 0, 100, 1, false, null); + + $service = $this->createMock(LeantimeApiService::class); + $service->method('updateAsJob')->willThrowException(new \RuntimeException('the database went away')); + + $handler = new LeantimeUpdateHandler($this->createMock(LoggerInterface::class), $service); + + // Dropping this page would also drop every page after it, since each queues the next. + try { + $handler($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } } diff --git a/tests/Unit/MessageHandler/UpsertIssueHandlerTest.php b/tests/Unit/MessageHandler/UpsertIssueHandlerTest.php index ae6da3cb..23dc6f47 100644 --- a/tests/Unit/MessageHandler/UpsertIssueHandlerTest.php +++ b/tests/Unit/MessageHandler/UpsertIssueHandlerTest.php @@ -3,6 +3,7 @@ namespace App\Tests\Unit\MessageHandler; use App\Enum\IssueStatusEnum; +use App\Exception\NotFoundException; use App\Message\UpsertIssueMessage; use App\MessageHandler\UpsertIssueHandler; use App\Model\DataProvider\DataProviderIssueData; @@ -33,16 +34,35 @@ public function testInvokeCallsUpsertIssue(): void $handler($message); } - public function testInvokeOnExceptionThrowsUnrecoverable(): void + public function testInvokeOnRowLevelFailureThrowsUnrecoverable(): void { $message = new UpsertIssueMessage($this->createIssueData()); $service = $this->createMock(DataProviderService::class); - $service->method('upsertIssue')->willThrowException(new \RuntimeException('fail')); + $service->method('upsertIssue')->willThrowException(new NotFoundException('fail')); $handler = new UpsertIssueHandler($this->createMock(LoggerInterface::class), $service); $this->expectException(UnrecoverableMessageHandlingException::class); $handler($message); } + + public function testInvokeOnInfrastructureFailurePropagates(): void + { + $message = new UpsertIssueMessage($this->createIssueData()); + + $service = $this->createMock(DataProviderService::class); + $service->method('upsertIssue')->willThrowException(new \RuntimeException('the database went away')); + + $handler = new UpsertIssueHandler($this->createMock(LoggerInterface::class), $service); + + // Unrecoverable is what LeantimeApiService reads as "bad row, carry on". This is not that. + try { + $handler($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } } diff --git a/tests/Unit/MessageHandler/UpsertProjectHandlerTest.php b/tests/Unit/MessageHandler/UpsertProjectHandlerTest.php index c8589c8b..40e786ed 100644 --- a/tests/Unit/MessageHandler/UpsertProjectHandlerTest.php +++ b/tests/Unit/MessageHandler/UpsertProjectHandlerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Unit\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertProjectMessage; use App\MessageHandler\UpsertProjectHandler; use App\Model\DataProvider\DataProviderProjectData; @@ -24,17 +25,37 @@ public function testInvokeCallsUpsertProject(): void $handler($message); } - public function testInvokeOnExceptionThrowsUnrecoverable(): void + public function testInvokeOnRowLevelFailureThrowsUnrecoverable(): void { $data = new DataProviderProjectData(1, 'Test', 'PT-1', 'http://test', new \DateTime(), new \DateTime()); $message = new UpsertProjectMessage($data); $service = $this->createMock(DataProviderService::class); - $service->method('upsertProject')->willThrowException(new \RuntimeException('fail')); + $service->method('upsertProject')->willThrowException(new NotFoundException('fail')); $handler = new UpsertProjectHandler($this->createMock(LoggerInterface::class), $service); $this->expectException(UnrecoverableMessageHandlingException::class); $handler($message); } + + public function testInvokeOnInfrastructureFailurePropagates(): void + { + $data = new DataProviderProjectData(1, 'Test', 'PT-1', 'http://test', new \DateTime(), new \DateTime()); + $message = new UpsertProjectMessage($data); + + $service = $this->createMock(DataProviderService::class); + $service->method('upsertProject')->willThrowException(new \RuntimeException('the database went away')); + + $handler = new UpsertProjectHandler($this->createMock(LoggerInterface::class), $service); + + // Unrecoverable is what LeantimeApiService reads as "bad row, carry on". This is not that. + try { + $handler($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } } diff --git a/tests/Unit/MessageHandler/UpsertVersionHandlerTest.php b/tests/Unit/MessageHandler/UpsertVersionHandlerTest.php index ea83c3ed..ee600259 100644 --- a/tests/Unit/MessageHandler/UpsertVersionHandlerTest.php +++ b/tests/Unit/MessageHandler/UpsertVersionHandlerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Unit\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertVersionMessage; use App\MessageHandler\UpsertVersionHandler; use App\Model\DataProvider\DataProviderVersionData; @@ -24,17 +25,37 @@ public function testInvokeCallsUpsertVersion(): void $handler($message); } - public function testInvokeOnExceptionThrowsUnrecoverable(): void + public function testInvokeOnRowLevelFailureThrowsUnrecoverable(): void { $data = new DataProviderVersionData(1, 'v1.0', 'VER-1', 'PT-1', new \DateTime(), new \DateTime()); $message = new UpsertVersionMessage($data); $service = $this->createMock(DataProviderService::class); - $service->method('upsertVersion')->willThrowException(new \RuntimeException('fail')); + $service->method('upsertVersion')->willThrowException(new NotFoundException('fail')); $handler = new UpsertVersionHandler($this->createMock(LoggerInterface::class), $service); $this->expectException(UnrecoverableMessageHandlingException::class); $handler($message); } + + public function testInvokeOnInfrastructureFailurePropagates(): void + { + $data = new DataProviderVersionData(1, 'v1.0', 'VER-1', 'PT-1', new \DateTime(), new \DateTime()); + $message = new UpsertVersionMessage($data); + + $service = $this->createMock(DataProviderService::class); + $service->method('upsertVersion')->willThrowException(new \RuntimeException('the database went away')); + + $handler = new UpsertVersionHandler($this->createMock(LoggerInterface::class), $service); + + // Unrecoverable is what LeantimeApiService reads as "bad row, carry on". This is not that. + try { + $handler($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } } diff --git a/tests/Unit/MessageHandler/UpsertWorkerHandlerTest.php b/tests/Unit/MessageHandler/UpsertWorkerHandlerTest.php index 616f3f2f..6d675b15 100644 --- a/tests/Unit/MessageHandler/UpsertWorkerHandlerTest.php +++ b/tests/Unit/MessageHandler/UpsertWorkerHandlerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Unit\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertWorkerMessage; use App\MessageHandler\UpsertWorkerHandler; use App\Model\DataProvider\DataProviderWorkerData; @@ -24,17 +25,37 @@ public function testInvokeCallsUpsertWorker(): void $handler($message); } - public function testInvokeOnExceptionThrowsUnrecoverable(): void + public function testInvokeOnRowLevelFailureThrowsUnrecoverable(): void { $data = new DataProviderWorkerData(1, 'John', 'john@test.com'); $message = new UpsertWorkerMessage($data); $service = $this->createMock(DataProviderService::class); - $service->method('upsertWorker')->willThrowException(new \RuntimeException('fail')); + $service->method('upsertWorker')->willThrowException(new NotFoundException('fail')); $handler = new UpsertWorkerHandler($this->createMock(LoggerInterface::class), $service); $this->expectException(UnrecoverableMessageHandlingException::class); $handler($message); } + + public function testInvokeOnInfrastructureFailurePropagates(): void + { + $data = new DataProviderWorkerData(1, 'John', 'john@test.com'); + $message = new UpsertWorkerMessage($data); + + $service = $this->createMock(DataProviderService::class); + $service->method('upsertWorker')->willThrowException(new \RuntimeException('the database went away')); + + $handler = new UpsertWorkerHandler($this->createMock(LoggerInterface::class), $service); + + // Unrecoverable is what LeantimeApiService reads as "bad row, carry on". This is not that. + try { + $handler($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } } diff --git a/tests/Unit/MessageHandler/UpsertWorklogHandlerTest.php b/tests/Unit/MessageHandler/UpsertWorklogHandlerTest.php index e82b5601..e41c1cdc 100644 --- a/tests/Unit/MessageHandler/UpsertWorklogHandlerTest.php +++ b/tests/Unit/MessageHandler/UpsertWorklogHandlerTest.php @@ -2,6 +2,7 @@ namespace App\Tests\Unit\MessageHandler; +use App\Exception\NotFoundException; use App\Message\UpsertWorklogMessage; use App\MessageHandler\UpsertWorklogHandler; use App\Model\DataProvider\DataProviderWorklogData; @@ -31,16 +32,35 @@ public function testInvokeCallsUpsertWorklog(): void $handler($message); } - public function testInvokeOnExceptionThrowsUnrecoverable(): void + public function testInvokeOnRowLevelFailureThrowsUnrecoverable(): void { $message = new UpsertWorklogMessage($this->createWorklogData()); $service = $this->createMock(DataProviderService::class); - $service->method('upsertWorklog')->willThrowException(new \RuntimeException('fail')); + $service->method('upsertWorklog')->willThrowException(new NotFoundException('fail')); $handler = new UpsertWorklogHandler($this->createMock(LoggerInterface::class), $service); $this->expectException(UnrecoverableMessageHandlingException::class); $handler($message); } + + public function testInvokeOnInfrastructureFailurePropagates(): void + { + $message = new UpsertWorklogMessage($this->createWorklogData()); + + $service = $this->createMock(DataProviderService::class); + $service->method('upsertWorklog')->willThrowException(new \RuntimeException('the database went away')); + + $handler = new UpsertWorklogHandler($this->createMock(LoggerInterface::class), $service); + + // Unrecoverable is what LeantimeApiService reads as "bad row, carry on". This is not that. + try { + $handler($message); + $this->fail('Expected the failure to propagate.'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf(UnrecoverableMessageHandlingException::class, $e); + $this->assertSame('the database went away', $e->getMessage()); + } + } }