fix: stop the Leantime sync halting silently on a bad row - #325
Conversation
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.
There was a problem hiding this comment.
Reviewed locally. Full suite passes in phpfpm: 395 tests, php-cs-fixer and PHPStan clean, coverage 64.54 % (threshold 62 %).
Direction is right, and the deletedAfter → deleted fix is a real bug caught. My comments share one theme: both mechanisms here trade a loud failure for a quiet one, and in places the quiet outcome is wrong data rather than a skipped row.
Would want resolved before merge:
catch (\Throwable)around the dispatch turns infrastructure failures into row skips while the command exitsSUCCESS.deleteAsJob()is still unguarded, and honouringdeletedremoves the full-history re-fetch that made a skipped deletion self-heal.- The placeholders corrupt data:
'(no name)'collides across entities in report aggregation,deleted-user-<userId>overwrites correct worker attribution.
One finding has no inline anchor because the code is untouched — the point of it. getWorkerUpsertFromResult() (LeantimeApiService.php:336) is the mapper the null-safety pass missed: no ?? self::NAME_MISSING, no coverage in testUpdateWithNullValues(). When data-api#18 makes name nullable, the TypeError is swallowed by the widened catch and the worker is never created. It also passes $result->id where dataProviderId is expected, ignoring the parameter.
Rest is smaller: missing test for the deleted fix, unrelated Taskfile.yml change, eight-way duplicated catch body, inconsistent log levels.
| [new TransportNamesStamp($asyncJobQueue ? $this::QUEUE_ASYNC : $this::QUEUE_SYNC)], | ||
| ); | ||
| } | ||
| } catch (\Throwable $e) { |
There was a problem hiding this comment.
\Throwable here does not separate "bad row" from "database went away". If an upsert flush closes the EntityManager, this logs Skipping ..., line 214 still queues the next page, and every remaining row fails the same way — app:data-providers:sync exits SUCCESS having imported nothing.
Catch \TypeError/NotAcceptableException around the mapping instead, and let infrastructure failures propagate.
| @@ -137,6 +142,13 @@ public function deleteAsJob(int $dataProviderId, bool $asyncJobQueue = false, ?\ | |||
| }; | |||
|
|
|||
| foreach ($results->{$type} as $result) { | |||
There was a problem hiding this comment.
This loop got the null-id guard but no try/catch. SyncDeletedCommand passes asyncJobQueue=false, so the handler runs inline and its UnrecoverableMessageHandlingException escapes here — one bad entry drops every remaining deletion in the run.
It used to self-heal because deletedAfter was ignored and each run re-pulled everything. With deleted honoured (good fix), a skipped deletion is permanent.
Same per-row try/catch as dispatchUpsertMessage().
| $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'), |
There was a problem hiding this comment.
This overwrites a correct value. upsertWorklog() calls setWorker('deleted-user-42') (DataProviderService.php:248), replacing a stored jane@example.com — her hours leave every worker-scoped report, and the rolling modifiedAfter window means it never comes back. With userId also null, different people merge into one deleted-user-unknown.
Only apply the fallback when worker is not already set.
| return new DataProviderProjectData( | ||
| $dataProviderId, | ||
| $result->name, | ||
| $result->name ?? self::NAME_MISSING, |
There was a problem hiding this comment.
Two nameless projects both become (no name) (same at line 290 for issues). BillableUnbilledHoursReportService and CybersecurityReportService key their aggregates on the name string, so unrelated projects merge into one row with summed hours.
Storing instead of failing is right — make it unique: sprintf('(no name) %s', $projectTrackerId).
| @@ -267,7 +287,7 @@ private function getIssueUpsertFromResult(object $result, int $dataProviderId, \ | |||
| $projectTrackerId, | |||
| $dataProviderId, | |||
| (string) $result->projectId, | |||
There was a problem hiding this comment.
getVersionUpsertFromResult() now throws on a null projectId (line 267); this still casts it to ''. getProject('') returns null, Issue::$project is nullable, so the issue is stored with no project — and an existing association gets wiped. CybersecurityReportService.php:60-61 then fatals on getProject()->getId().
The worklog fixture (id 101) already has "projectId": null. Mirror the version guard.
| '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(), |
There was a problem hiding this comment.
Good catch — and the only change here without a test. The client is mocked with ->method('request')->willReturn(...), so the request body is never asserted: re-introducing the typo keeps the suite green while every delete-sync re-downloads the full unpaginated history.
Pin it with ->with() on the params passed to post().
| 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)); |
There was a problem hiding this comment.
warning() here vs error() at line 245 for the same event — a row that cannot be mapped and is dropped. After data-api#18, routine null rows will page anyone alerting on error while skipped deletions stay invisible.
One level for both paths.
| $this->logger->info('Upserting issue: '.$message->issueData->name); | ||
| $this->dataProviderService->upsertIssue($message->issueData); | ||
| } catch (\Exception $e) { | ||
| } catch (\Throwable $e) { |
There was a problem hiding this comment.
Eight handlers carry a byte-identical catch body, so \Exception → \Throwable took eight identical diffs — and a handler added later can silently diverge.
Not blocking. Follow-up: a Messenger middleware or a shared trait.
| # 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" }}' |
There was a problem hiding this comment.
Unrelated to the sync fix. Flips the default docker wrapper to docker compose for every developer, and line 150 switches fixtures from hautelook to doctrine — no CHANGELOG entry, shipping under a PR titled as a sync fix.
Own PR.
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-<userId> 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.
2 participants
Link to ticket
https://leantime.itkdev.dk/#/tickets/showTicket/8000
Description
LeantimeApiServiceand the sync message handlers now catch\Throwablerather than\Exception, so aTypeErrorfrom a nullable source field no longer escapes uncaught. The upsert dispatch moved inside the sametry, because on thesynctransport the handler runs inline and its failure previously escaped the row loop inupdateAsJob()before the next page was queued. A skipped row now logsSkipping <class> id <id>: <reason>and the sync continues.username,kind,ticketId,projectIdandnamenullable and addsuserId. A worklog whose Leantime user was deleted keeps its hours and is attributed todeleted-user-<userId>; a missing project, version or issue name becomes(no name)rather than failing to store; a null ticket status maps toIssueStatusEnum::OTHER; and nullplannedHours/remainingHoursare allowed through. Timesheets with noticketIdand milestones with noprojectIdare skipped and logged, sinceWorklog::$issueandVersion::$projectcannot be null./deletedrequest sending its timestamp asdeletedAfter, which the Leantime plugin ignores — it readsdeleted. 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.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: aTypeErrorand a failure raised inside the upsert handler. The/deletedfixture gained an entry with no id.Checklist