From ebb146b96a451d1a399ce9f08c838e840b9b58be Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Thu, 30 Jul 2026 11:11:56 +0000 Subject: [PATCH 1/4] fix: stop the Leantime sync halting silently on a bad row LeantimeApiService and the sync message handlers caught \Exception, so a TypeError from a nullable source field mapped onto a non-nullable constructor argument escaped uncaught. It left the row loop in updateAsJob() before the next page was queued, stopping the sync without a visible error. Catch \Throwable instead. The upsert dispatch moves inside the same try: on the sync transport the handler runs inline during dispatch(), so its failure previously escaped the row loop the same way. A skipped row now logs the class and id. Prepares economics for ITK-Leantime/data-api#18, which makes username, kind, ticketId, projectId and name nullable in the API response. --- CHANGELOG.md | 8 +++++++ .../EntityRemovedFromDataProviderHandler.php | 2 +- src/MessageHandler/LeantimeDeleteHandler.php | 2 +- src/MessageHandler/LeantimeUpdateHandler.php | 2 +- src/MessageHandler/UpsertIssueHandler.php | 2 +- src/MessageHandler/UpsertProjectHandler.php | 2 +- src/MessageHandler/UpsertVersionHandler.php | 2 +- src/MessageHandler/UpsertWorkerHandler.php | 2 +- src/MessageHandler/UpsertWorklogHandler.php | 2 +- src/Service/LeantimeApiService.php | 23 +++++++++++-------- 10 files changed, 29 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c6b8547..30814887 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ 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. `LeantimeApiService` + and the sync message handlers now catch `\Throwable` rather than `\Exception`, so a + `TypeError` from a nullable source field no longer escapes uncaught. The upsert + dispatch moved inside the same `try`, because on the `sync` transport the handler + runs inline and its failure previously escaped the row loop in `updateAsJob()` + before the next page was queued. A skipped row now logs + `Skipping id : ` and the sync continues. * [PR-324](https://github.com/itk-dev/economics/pull/324) Added game center with snake * [PR-303](https://github.com/itk-dev/economics/pull/303) diff --git a/src/MessageHandler/EntityRemovedFromDataProviderHandler.php b/src/MessageHandler/EntityRemovedFromDataProviderHandler.php index 91da47c1..c00b715a 100644 --- a/src/MessageHandler/EntityRemovedFromDataProviderHandler.php +++ b/src/MessageHandler/EntityRemovedFromDataProviderHandler.php @@ -34,7 +34,7 @@ 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 (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/LeantimeDeleteHandler.php b/src/MessageHandler/LeantimeDeleteHandler.php index 0f63c5b2..018e2867 100644 --- a/src/MessageHandler/LeantimeDeleteHandler.php +++ b/src/MessageHandler/LeantimeDeleteHandler.php @@ -27,7 +27,7 @@ public function __invoke(LeantimeDeleteMessage $message): void $message->asyncJobQueue, $message->deletedAfter, ); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/LeantimeUpdateHandler.php b/src/MessageHandler/LeantimeUpdateHandler.php index b8cb5a0e..a00d9e90 100644 --- a/src/MessageHandler/LeantimeUpdateHandler.php +++ b/src/MessageHandler/LeantimeUpdateHandler.php @@ -32,7 +32,7 @@ public function __invoke(LeantimeUpdateMessage $message): void $message->modifiedAfter, $message->disableModifiedAtCheck, ); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertIssueHandler.php b/src/MessageHandler/UpsertIssueHandler.php index 47a4d3f6..fdb49d0c 100644 --- a/src/MessageHandler/UpsertIssueHandler.php +++ b/src/MessageHandler/UpsertIssueHandler.php @@ -22,7 +22,7 @@ public function __invoke(UpsertIssueMessage $message): void try { $this->logger->info('Upserting issue: '.$message->issueData->name); $this->dataProviderService->upsertIssue($message->issueData); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertProjectHandler.php b/src/MessageHandler/UpsertProjectHandler.php index 5e1e0564..4c422f4e 100644 --- a/src/MessageHandler/UpsertProjectHandler.php +++ b/src/MessageHandler/UpsertProjectHandler.php @@ -22,7 +22,7 @@ public function __invoke(UpsertProjectMessage $message): void try { $this->logger->info('Upserting project: '.$message->projectData->name); $this->dataProviderService->upsertProject($message->projectData); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertVersionHandler.php b/src/MessageHandler/UpsertVersionHandler.php index 71a91460..4c743ed1 100644 --- a/src/MessageHandler/UpsertVersionHandler.php +++ b/src/MessageHandler/UpsertVersionHandler.php @@ -22,7 +22,7 @@ public function __invoke(UpsertVersionMessage $message): void try { $this->logger->info('Upserting version: '.$message->versionData->name); $this->dataProviderService->upsertVersion($message->versionData); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertWorkerHandler.php b/src/MessageHandler/UpsertWorkerHandler.php index a3b9e247..ba726cea 100644 --- a/src/MessageHandler/UpsertWorkerHandler.php +++ b/src/MessageHandler/UpsertWorkerHandler.php @@ -22,7 +22,7 @@ public function __invoke(UpsertWorkerMessage $message): void try { $this->logger->info('Upserting worker: '.$message->workerData->email); $this->dataProviderService->upsertWorker($message->workerData); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/MessageHandler/UpsertWorklogHandler.php b/src/MessageHandler/UpsertWorklogHandler.php index 50312a39..44364bb2 100644 --- a/src/MessageHandler/UpsertWorklogHandler.php +++ b/src/MessageHandler/UpsertWorklogHandler.php @@ -22,7 +22,7 @@ public function __invoke(UpsertWorklogMessage $message): void try { $this->logger->info('Upserting worklog: '.$message->worklogData->projectTrackerId); $this->dataProviderService->upsertWorklog($message->worklogData); - } catch (\Exception $e) { + } catch (\Throwable $e) { $this->logger->error($e->getMessage()); throw new UnrecoverableMessageHandlingException($e->getMessage()); } diff --git a/src/Service/LeantimeApiService.php b/src/Service/LeantimeApiService.php index 1aff412b..1e9363f5 100644 --- a/src/Service/LeantimeApiService.php +++ b/src/Service/LeantimeApiService.php @@ -208,6 +208,11 @@ 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 { + // Catch \Throwable, not \Exception: a nullable source field mapped onto a non-nullable + // constructor argument raises a TypeError, which extends Error. Uncaught, it escapes the + // row loop in updateAsJob() before the next page is queued, halting the sync silently. + // The dispatch belongs inside the try for the same reason: on the sync transport the + // handler runs inline here, so its failures surface as part of this call. try { $message = match ($className) { Project::class => new UpsertProjectMessage($this->getProjectUpsertFromResult($data, $dataProviderId, $fetchDate, $dataProviderUrl, $disableModifiedAtCheck)), @@ -217,17 +222,15 @@ 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()); - return; - } - - if (null !== $message) { - $this->messageBus->dispatch( - $message, - [new TransportNamesStamp($asyncJobQueue ? $this::QUEUE_ASYNC : $this::QUEUE_SYNC)], - ); + if (null !== $message) { + $this->messageBus->dispatch( + $message, + [new TransportNamesStamp($asyncJobQueue ? $this::QUEUE_ASYNC : $this::QUEUE_SYNC)], + ); + } + } catch (\Throwable $e) { + $this->logger->error(sprintf('Skipping %s id %s: %s', $className, $data->id ?? '?', $e->getMessage())); } } From b7c001a4368971aa5e7148a59a8ae06a918122f5 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Fri, 31 Jul 2026 07:30:57 +0200 Subject: [PATCH 2/4] fix: handled null values --- CHANGELOG.md | 31 +- Taskfile.yml | 4 +- phpstan-baseline.neon | 26 +- .../DataProvider/DataProviderIssueData.php | 4 +- .../DataProvider/DataProviderWorklogData.php | 2 +- src/Service/DataProviderService.php | 2 +- src/Service/LeantimeApiService.php | 36 ++- .../Service/LeantimeApiServiceTest.php | 287 ++++++++++++++++++ 8 files changed, 357 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30814887..1b0632ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,13 +9,30 @@ 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. `LeantimeApiService` - and the sync message handlers now catch `\Throwable` rather than `\Exception`, so a - `TypeError` from a nullable source field no longer escapes uncaught. The upsert - dispatch moved inside the same `try`, because on the `sync` transport the handler - runs inline and its failure previously escaped the row loop in `updateAsJob()` - before the next page was queued. A skipped row now logs - `Skipping id : ` and the sync continues. + * Fixed the Leantime sync halting silently on a single bad row. `LeantimeApiService` + and the sync message handlers now catch `\Throwable` rather than `\Exception`, so a + `TypeError` from a nullable source field no longer escapes uncaught. The upsert + dispatch moved inside the same `try`, because on the `sync` transport the handler + runs inline and its failure previously escaped the row loop in `updateAsJob()` + before the next page was queued. A skipped row now logs + `Skipping id : ` and the sync continues. + * Made the Leantime result mappers null-safe, ahead of + [data-api#18](https://github.com/ITK-Leantime/data-api/pull/18) which makes + `username`, `kind`, `ticketId`, `projectId` and `name` nullable and adds `userId`. + A worklog whose Leantime user was deleted keeps its hours and is attributed to + `deleted-user-`; a missing project, version or issue name becomes + `(no name)` rather than failing to store; a null ticket status maps to + `IssueStatusEnum::OTHER`; and null `plannedHours`/`remainingHours` are allowed + through. Timesheets with no `ticketId` and milestones with no `projectId` are + skipped and logged, since `Worklog::$issue` and `Version::$project` cannot be null. + * Fixed the `/deleted` request sending its timestamp as `deletedAfter`, which the Leantime + plugin ignores — it reads `deleted`. Every delete-sync was pulling the entire deletion + history, on an endpoint the plugin does not paginate. Deletion entries with no id are + now skipped and logged rather than aborting the remaining types. + * Added `LeantimeApiServiceTest::testUpdateWithNullValues()`, covering the nullable payload + from data-api#18 plus two probes that a single unmappable row is logged and skipped + rather than stopping the sync: a `TypeError` and a failure raised inside the upsert + handler. The `/deleted` fixture gained an entry with no id. * [PR-324](https://github.com/itk-dev/economics/pull/324) Added game center with snake * [PR-303](https://github.com/itk-dev/economics/pull/303) diff --git a/Taskfile.yml b/Taskfile.yml index 9e884b96..47b61dd4 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -9,7 +9,7 @@ dotenv: [".env.local", ".env"] vars: # https://taskfile.dev/reference/templating/ BASE_URL: "{{.TASK_BASE_URL | default .COMPOSE_SERVER_DOMAIN | default .COMPOSE_DOMAIN }}" - DOCKER_COMPOSE: '{{ .TASK_DOCKER_COMPOSE | default "itkdev-docker-compose" }}' + DOCKER_COMPOSE: '{{ .TASK_DOCKER_COMPOSE | default "docker compose" }}' tasks: default: @@ -147,7 +147,7 @@ tasks: prompt: "This will reset fixture data. Continue?" desc: Load data fixtures. cmds: - - task composer -- fixtures:load + - task phpfpm -- bin/console doctrine:fixtures:load --no-interaction # ----------------------------------------------------------- Messenger --- diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index d1208155..8e2cf07d 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -2056,7 +2056,7 @@ parameters: - message: '#^Access to an undefined property object\:\:\$name\.$#' identifier: property.notFound - count: 4 + count: 1 path: src/Service/LeantimeApiService.php - @@ -2113,12 +2113,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 +2926,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: 17 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Call to an undefined method object\:\:flush\(\)\.$#' identifier: method.notFound - count: 3 + count: 4 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Call to an undefined method object\:\:persist\(\)\.$#' identifier: method.notFound - count: 12 + count: 13 path: tests/Integration/Service/LeantimeApiServiceTest.php - @@ -3028,31 +3022,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: 3 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: 3 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: 12 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: 3 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: 3 path: tests/Integration/Service/LeantimeApiServiceTest.php - 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..a66b8c08 100644 --- a/src/Model/DataProvider/DataProviderWorklogData.php +++ b/src/Model/DataProvider/DataProviderWorklogData.php @@ -12,7 +12,7 @@ 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, diff --git a/src/Service/DataProviderService.php b/src/Service/DataProviderService.php index 899f55c5..50c677d2 100644 --- a/src/Service/DataProviderService.php +++ b/src/Service/DataProviderService.php @@ -251,7 +251,7 @@ public function upsertWorklog(DataProviderWorklogData $upsertWorklogData): void $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 1e9363f5..99256e3f 100644 --- a/src/Service/LeantimeApiService.php +++ b/src/Service/LeantimeApiService.php @@ -43,6 +43,9 @@ 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. + private const NAME_MISSING = '(no name)'; private const QUEUE_ASYNC = 'async'; private const QUEUE_SYNC = 'sync'; @@ -116,7 +119,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,6 +142,13 @@ public function deleteAsJob(int $dataProviderId, bool $asyncJobQueue = false, ?\ }; foreach ($results->{$type} as $result) { + // Nothing identifies the entity to remove, and this loop has no other guard. + if (null === $result->id) { + $this->logger->warning(sprintf('Skipping deleted %s entry with no id', $type)); + + continue; + } + $projectTrackerId = $result->id; $deletedDate = $this->getLeanDateTime($result->deletedDate); @@ -240,7 +252,7 @@ private function getProjectUpsertFromResult(object $result, int $dataProviderId, return new DataProviderProjectData( $dataProviderId, - $result->name, + $result->name ?? self::NAME_MISSING, $projectTrackerId, $this->linkToProject($projectTrackerId, $dataProviderUrl), $fetchDate, @@ -251,9 +263,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 ?? self::NAME_MISSING, (string) $result->id, (string) $result->projectId, $fetchDate, @@ -270,7 +287,7 @@ private function getIssueUpsertFromResult(object $result, int $dataProviderId, \ $projectTrackerId, $dataProviderId, (string) $result->projectId, - $result->name, + $result->name ?? self::NAME_MISSING, $result->tags, $result->plannedHours, $result->remainingHours, @@ -294,13 +311,20 @@ 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'); + } + return new DataProviderWorklogData( $result->id, $dataProviderId, (string) $result->ticketId, $result->description, $startedDate, - $result->username, + // 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. + $result->username ?? 'deleted-user-'.($result->userId ?? 'unknown'), $result->hours, $result->kind, $fetchDate, @@ -346,7 +370,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..90fc73cb 100644 --- a/tests/Integration/Service/LeantimeApiServiceTest.php +++ b/tests/Integration/Service/LeantimeApiServiceTest.php @@ -159,6 +159,117 @@ 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. + $before = count($projectRepository->findAll()); + $service->updateAsJob(Project::class, 0, 100, $dataProvider->getId()); + $this->assertEquals($before + 2, count($projectRepository->findAll())); + $this->assertEquals('(no name)', $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)', $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. + $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)', $issue->getName()); + $this->assertEquals(IssueStatusEnum::OTHER, $issue->getStatus()); + $this->assertNull($issue->getPlanHours()); + + // 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(4, $loggedErrors); + $this->assertStringContainsString('projectId is null', $loggedErrors[0]); + $this->assertStringContainsString('ticketId is null', $loggedErrors[1]); + $this->assertStringContainsString('Skipping App\Entity\Worklog id 103', $loggedErrors[2]); + $this->assertStringContainsString('999', $loggedErrors[3]); + } + public function testDeleted(): void { self::bootKernel(); @@ -370,6 +481,176 @@ 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": 2, + "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" + } + ] + } + ', 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 getDeletedData(): object { return json_decode(' @@ -415,6 +696,10 @@ private function getDeletedData(): object } ], "timesheets": [ + { + "id": null, + "deletedDate": "2025-10-24T11:36:08.000000Z" + }, { "id": 66937, "deletedDate": "2025-10-24T11:36:08.000000Z" @@ -540,6 +825,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 +837,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", From 75d8de915e5955c2f720577181bcfab9dcc70b15 Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Tue, 4 Aug 2026 08:51:03 +0200 Subject: [PATCH 3/4] chore: cleaned up pr entries --- CHANGELOG.md | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b0632ab..92627fb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,30 +9,16 @@ 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. `LeantimeApiService` - and the sync message handlers now catch `\Throwable` rather than `\Exception`, so a - `TypeError` from a nullable source field no longer escapes uncaught. The upsert - dispatch moved inside the same `try`, because on the `sync` transport the handler - runs inline and its failure previously escaped the row loop in `updateAsJob()` - before the next page was queued. A skipped row now logs + * Fixed the Leantime sync halting silently on a single bad row: `LeantimeApiService` and the sync message + handlers now catch `\Throwable`, and the upsert dispatch moved inside the same `try`. A skipped row logs `Skipping id : ` and the sync continues. - * Made the Leantime result mappers null-safe, ahead of - [data-api#18](https://github.com/ITK-Leantime/data-api/pull/18) which makes - `username`, `kind`, `ticketId`, `projectId` and `name` nullable and adds `userId`. - A worklog whose Leantime user was deleted keeps its hours and is attributed to - `deleted-user-`; a missing project, version or issue name becomes - `(no name)` rather than failing to store; a null ticket status maps to - `IssueStatusEnum::OTHER`; and null `plannedHours`/`remainingHours` are allowed - through. Timesheets with no `ticketId` and milestones with no `projectId` are - skipped and logged, since `Worklog::$issue` and `Version::$project` cannot be null. - * Fixed the `/deleted` request sending its timestamp as `deletedAfter`, which the Leantime - plugin ignores — it reads `deleted`. Every delete-sync was pulling the entire deletion - history, on an endpoint the plugin does not paginate. Deletion entries with no id are - now skipped and logged rather than aborting the remaining types. - * Added `LeantimeApiServiceTest::testUpdateWithNullValues()`, covering the nullable payload - from data-api#18 plus two probes that a single unmappable row is logged and skipped - rather than stopping the sync: a `TypeError` and a failure raised inside the upsert - handler. The `/deleted` fixture gained an entry with no id. + * 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` are skipped. + * 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. + * Added `LeantimeApiServiceTest::testUpdateWithNullValues()`, covering the nullable payload plus probes that a + single unmappable row is logged and skipped rather than stopping the sync. * [PR-324](https://github.com/itk-dev/economics/pull/324) Added game center with snake * [PR-303](https://github.com/itk-dev/economics/pull/303) From dd761f8b6b71859caf68fa8e6bf14bbfbd88df3a Mon Sep 17 00:00:00 2001 From: Troels Ugilt Jensen <6103205+tuj@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:50:12 +0000 Subject: [PATCH 4/4] fix: addressed review feedback on the Leantime sync Narrowed the exception handling so a bad row and a dead database stop being the same thing. The eight message handlers now mark only row-level failures as unrecoverable and let everything else propagate, which is what lets LeantimeApiService tell the two apart: it skips a row whose causes are all unrecoverable and rethrows otherwise. Fixed three ways the sync could lose or corrupt data: - A null projectId on an issue was cast to '', which looked up no project and silently cleared the association an existing issue already had, taking its worklogs' project with it. Now skipped. - deleteAsJob dispatched without a guard, so one bad entry dropped every deletion after it. Now that the request actually filters by timestamp those entries never come round again, so the loop guards each entry. - A deleted-user- attribution overwrote a worker name an earlier sync had stored. It now only fills an empty one. Also made the worker mapper null-safe, guarded the ids used for pagination and worklog lookup, gave both skip paths one log level, and made the (no name) placeholder unique per tracker id, since names are used as lookup keys elsewhere. Pinned the /deleted request body in a test so the deletedAfter -> deleted fix cannot regress, and reverted the unrelated Taskfile.yml changes. --- CHANGELOG.md | 20 +- Taskfile.yml | 4 +- phpstan-baseline.neon | 37 ++-- .../EntityRemovedFromDataProviderHandler.php | 4 +- src/MessageHandler/LeantimeDeleteHandler.php | 4 +- src/MessageHandler/LeantimeUpdateHandler.php | 5 +- src/MessageHandler/UpsertIssueHandler.php | 6 +- src/MessageHandler/UpsertProjectHandler.php | 4 +- src/MessageHandler/UpsertVersionHandler.php | 4 +- src/MessageHandler/UpsertWorkerHandler.php | 4 +- src/MessageHandler/UpsertWorklogHandler.php | 4 +- .../DataProvider/DataProviderWorklogData.php | 3 + src/Service/DataProviderService.php | 8 +- src/Service/LeantimeApiService.php | 135 ++++++++++--- .../Service/LeantimeApiServiceTest.php | 182 ++++++++++++++++-- ...tityRemovedFromDataProviderHandlerTest.php | 23 ++- .../LeantimeDeleteHandlerTest.php | 24 ++- .../LeantimeUpdateHandlerTest.php | 24 ++- .../MessageHandler/UpsertIssueHandlerTest.php | 24 ++- .../UpsertProjectHandlerTest.php | 25 ++- .../UpsertVersionHandlerTest.php | 25 ++- .../UpsertWorkerHandlerTest.php | 25 ++- .../UpsertWorklogHandlerTest.php | 24 ++- 23 files changed, 522 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92627fb3..e2e3d4d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,16 +9,22 @@ 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: `LeantimeApiService` and the sync message - handlers now catch `\Throwable`, and the upsert dispatch moved inside the same `try`. A skipped row logs - `Skipping id : ` and the sync continues. + * 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` are skipped. + `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. - * Added `LeantimeApiServiceTest::testUpdateWithNullValues()`, covering the nullable payload plus probes that a - single unmappable row is logged and skipped rather than stopping the sync. + 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. * [PR-324](https://github.com/itk-dev/economics/pull/324) Added game center with snake * [PR-303](https://github.com/itk-dev/economics/pull/303) diff --git a/Taskfile.yml b/Taskfile.yml index 47b61dd4..9e884b96 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -9,7 +9,7 @@ dotenv: [".env.local", ".env"] vars: # https://taskfile.dev/reference/templating/ BASE_URL: "{{.TASK_BASE_URL | default .COMPOSE_SERVER_DOMAIN | default .COMPOSE_DOMAIN }}" - DOCKER_COMPOSE: '{{ .TASK_DOCKER_COMPOSE | default "docker compose" }}' + DOCKER_COMPOSE: '{{ .TASK_DOCKER_COMPOSE | default "itkdev-docker-compose" }}' tasks: default: @@ -147,7 +147,7 @@ tasks: prompt: "This will reset fixture data. Continue?" desc: Load data fixtures. cmds: - - task phpfpm -- bin/console doctrine:fixtures:load --no-interaction + - task composer -- fixtures:load # ----------------------------------------------------------- Messenger --- diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 8e2cf07d..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: 1 - path: src/Service/LeantimeApiService.php - - message: '#^Access to an undefined property object\:\:\$plannedHours\.$#' identifier: property.notFound @@ -2932,19 +2925,19 @@ parameters: - message: '#^Call to an undefined method object\:\:findOneBy\(\)\.$#' identifier: method.notFound - count: 17 + count: 19 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Call to an undefined method object\:\:flush\(\)\.$#' identifier: method.notFound - count: 4 + count: 5 path: tests/Integration/Service/LeantimeApiServiceTest.php - message: '#^Call to an undefined method object\:\:persist\(\)\.$#' identifier: method.notFound - count: 13 + count: 17 path: tests/Integration/Service/LeantimeApiServiceTest.php - @@ -2980,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 - @@ -2998,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 - @@ -3022,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: 3 + 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: 3 + 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: 12 + 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: 3 + 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: 3 + count: 4 path: tests/Integration/Service/LeantimeApiServiceTest.php - @@ -3148,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 c00b715a..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 (\Throwable $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 018e2867..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 (\Throwable $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 a00d9e90..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 (\Throwable $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 fdb49d0c..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 (\Throwable $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 4c422f4e..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 (\Throwable $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 4c743ed1..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 (\Throwable $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 ba726cea..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 (\Throwable $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 44364bb2..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 (\Throwable $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/DataProviderWorklogData.php b/src/Model/DataProvider/DataProviderWorklogData.php index a66b8c08..ae8d708c 100644 --- a/src/Model/DataProvider/DataProviderWorklogData.php +++ b/src/Model/DataProvider/DataProviderWorklogData.php @@ -16,6 +16,9 @@ public function __construct( 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 50c677d2..163315d4 100644 --- a/src/Service/DataProviderService.php +++ b/src/Service/DataProviderService.php @@ -247,7 +247,13 @@ 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); diff --git a/src/Service/LeantimeApiService.php b/src/Service/LeantimeApiService.php index 99256e3f..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; @@ -44,7 +46,9 @@ class LeantimeApiService implements DataProviderInterface 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. + // 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'; @@ -142,20 +146,32 @@ public function deleteAsJob(int $dataProviderId, bool $asyncJobQueue = false, ?\ }; foreach ($results->{$type} as $result) { - // Nothing identifies the entity to remove, and this loop has no other guard. + // Nothing identifies the entity to remove. if (null === $result->id) { - $this->logger->warning(sprintf('Skipping deleted %s entry with no id', $type)); + $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())); + } } } } @@ -199,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; @@ -220,11 +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 { - // Catch \Throwable, not \Exception: a nullable source field mapped onto a non-nullable - // constructor argument raises a TypeError, which extends Error. Uncaught, it escapes the - // row loop in updateAsJob() before the next page is queued, halting the sync silently. - // The dispatch belongs inside the try for the same reason: on the sync transport the - // handler runs inline here, so its failures surface as part of this call. + // 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)), @@ -234,25 +252,59 @@ private function dispatchUpsertMessage(string $className, object $data, int $dat Worker::class => new UpsertWorkerMessage($this->getWorkerUpsertFromResult($data, $dataProviderId, $fetchDate)), default => null, }; + } catch (NotAcceptableException|\TypeError $e) { + $this->logger->error(sprintf('Skipping %s id %s: %s', $className, $data->id ?? '?', $e->getMessage())); + + return; + } + + 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); - if (null !== $message) { - $this->messageBus->dispatch( - $message, - [new TransportNamesStamp($asyncJobQueue ? $this::QUEUE_ASYNC : $this::QUEUE_SYNC)], - ); - } - } catch (\Throwable $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; + } + } + } + private function getProjectUpsertFromResult(object $result, int $dataProviderId, \DateTimeInterface $fetchDate, ?string $dataProviderUrl = null, bool $disableModifiedAtCheck = false): DataProviderProjectData { $projectTrackerId = (string) $result->id; return new DataProviderProjectData( $dataProviderId, - $result->name ?? self::NAME_MISSING, + $result->name ?? $this->missingName($projectTrackerId), $projectTrackerId, $this->linkToProject($projectTrackerId, $dataProviderUrl), $fetchDate, @@ -270,7 +322,7 @@ private function getVersionUpsertFromResult(object $result, int $dataProviderId, return new DataProviderVersionData( $dataProviderId, - $result->name ?? self::NAME_MISSING, + $result->name ?? $this->missingName((string) $result->id), (string) $result->id, (string) $result->projectId, $fetchDate, @@ -281,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 ?? self::NAME_MISSING, + $result->name ?? $this->missingName($projectTrackerId), $result->tags, $result->plannedHours, $result->remainingHours, @@ -316,32 +375,52 @@ private function getWorklogUpsertFromResult(object $result, int $dataProviderId, 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, - // 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. - $result->username ?? 'deleted-user-'.($result->userId ?? 'unknown'), + $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); diff --git a/tests/Integration/Service/LeantimeApiServiceTest.php b/tests/Integration/Service/LeantimeApiServiceTest.php index 90fc73cb..df6ed0a2 100644 --- a/tests/Integration/Service/LeantimeApiServiceTest.php +++ b/tests/Integration/Service/LeantimeApiServiceTest.php @@ -218,27 +218,31 @@ function (string $message, array $context = []) use (&$loggedErrors) { $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)', $projectRepository->findOneBy(['projectTrackerId' => 70, 'dataProvider' => $dataProvider])->getName()); + $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)', $versionRepository->findOneBy(['projectTrackerId' => 20, 'dataProvider' => $dataProvider])->getName()); + $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. + // 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)', $issue->getName()); + $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 @@ -263,11 +267,98 @@ function (string $message, array $context = []) use (&$loggedErrors) { $this->assertNull($worklogRepository->findOneBy(['worklogId' => 104, 'dataProvider' => $dataProvider])); // Each skipped row is reported once, so a halt could never be silent. - $this->assertCount(4, $loggedErrors); - $this->assertStringContainsString('projectId is null', $loggedErrors[0]); - $this->assertStringContainsString('ticketId is null', $loggedErrors[1]); - $this->assertStringContainsString('Skipping App\Entity\Worklog id 103', $loggedErrors[2]); - $this->assertStringContainsString('999', $loggedErrors[3]); + $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 @@ -291,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, @@ -458,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. @@ -541,7 +654,7 @@ private function getNullValueTickets(): object "start": 0, "limit": 100 }, - "resultsCount": 2, + "resultsCount": 3, "results": [ { "id": 30, @@ -570,6 +683,20 @@ private function getNullValueTickets(): object "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" } ] } @@ -651,6 +778,33 @@ private function getNullValueTimesheets(): object ', 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(' @@ -700,6 +854,10 @@ private function getDeletedData(): object "id": null, "deletedDate": "2025-10-24T11:36:08.000000Z" }, + { + "id": 66939, + "deletedDate": "not a date" + }, { "id": 66937, "deletedDate": "2025-10-24T11:36:08.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()); + } + } }