feat: paginated the Leantime delete sync - #334
Conversation
/deleted now serves one type per request with start/limit (data-api#21), so the whole deletion history no longer has to arrive in a single response — which is what the 300s max_duration was sized for. delete() queues a message per type and deleteAsJob() pages through them the way updateAsJob() already does. The cursor is the endpoint's new deletionId, not the deleted entity's id, since deletions are ordered by when they happened. It advances past a deletion that names no entity — a skipped row still has to be paged past — and a full page with no usable deletionId stops with an error rather than re-queueing itself.
deleteAsJob() gained a match over the type and a second $data->resultsCount read, both of which the baseline already ignores once for updateAsJob(), and its $dataProviderId moved from the first parameter to the fourth.
data-api#21 renamed the parameter to match `modifiedAfter` on the entity endpoints. The old `deleted` now answers 400 rather than being ignored, so the key cannot go missing unnoticed the way it did before.
turegjorup
left a comment
There was a problem hiding this comment.
I don't know enough about the sync logic to know about the above comment. It it is a blocker or a trivial issue?
| public const TICKETS = 'tickets'; | ||
| public const TIMESHEETS = 'timesheets'; | ||
| public const WORKERS = 'workers'; | ||
| /** The types the deleted endpoint tracks, children before the parents they hang off. */ |
There was a problem hiding this comment.
This can not be guaranteed when running async. Depending on the entity model consider using doctrines orphan removal to also delete children when parents are deleted.
Or scope deletes by projects so that all deletes within a project happens as one message/job.
There was a problem hiding this comment.
You're right that the queue gives no ordering guarantee, and the comment was wrong to imply the
constant provides one. What actually holds the order here is that this path is not async:
SyncDeletedCommand calls deleteAll(false, …), so both the delete messages and the
EntityRemovedFromDataProviderMessages get TransportNamesStamp('sync') and every handler runs
inline — including the next-page dispatch, so a type's pages all finish before the next type
starts. I've rewritten the comment to say that, and to record that fanning the four types out up
front only works under inline handling: switching this path to async would mean chaining the types
instead.
On orphan removal — it would take billing data with it. projectRemovedFromDataProvider() and
issueRemovedFromDataProvider() refuse to hard-delete while invoice-bound children exist and set
sourceDeletedDate instead, and worklogRemovedFromDataProvider() protects any worklog attached
to an invoiceEntry. A cascade from the parent would delete exactly the rows those checks exist to
keep.
On scoping by project — /deleted pages by type over deletionId and has no project dimension, so
grouping by project would mean holding the whole deletion history in memory, which is what the
pagination is here to avoid.
Digging into this did turn up a real bug, unrelated to pagination:
projectRemovedFromDataProvider() checks invoices, issues and worklogs but not versions, and
version.project_id has no ON DELETE, so a project whose milestone deletion was missed passes the
removable check and then hits an FK violation. Fixing that separately.
Link to ticket
https://leantime.itkdev.dk/#/tickets/showTicket/8000
Description
The plugin's
/deletedendpoint is now paginated and serves one type per request(data-api#21), so this is the consumer
side of that change. It has to land together with the plugin:
types→typeand the flatresultsarray are both breaking, and the delete sync is broken in between either way.Until now
deleteAsJob()asked for all four types at once and read back an object keyed bytype, which meant a single response carrying the entire deletion history — the thing the
300s
max_durationinconfig/packages/framework.yamlwas sized for. That value stays asit is, since it also covers the entity endpoints, but nothing depends on it being that
generous any more.
The shape now mirrors
updateAsJob(), which is the point:delete()queues oneLeantimeDeleteMessageper type instead of one per data provider.The order is kept in a
DELETED_TYPESconstant — timesheets, tickets, milestones,projects — so children are removed before the parents they hang off.
LeantimeDeleteMessagegainstype,startandlimit, matchingLeantimeUpdateMessage.deleteAsJob(string $type, int $startId, int $limit, int $dataProviderId, …)fetches onepage, dispatches an
EntityRemovedFromDataProviderMessageper row, and queues the nextpage while
resultsCount === limit.The cursor is the endpoint's new
deletionId, not the deleted entity'sid. Deletionsare ordered by when they happened, so the entity ids on a page are in no particular order —
paging on them would skip deletions. Two consequences, both deliberate and both tested:
entity still occupies a place in the page, so it has to be paged past or the next request
refetches the same page.
deletionIdanywhere stops with an error rather thanre-queueing itself. This is the same guard, and the same reasoning, as
#326 added to
updateAsJob(): aninvisible infinite loop starves every other sync of the single worker, so stopping loudly
beats looping.
The narrow catches around the row loop from
#325 are untouched — a row-level failure
is still logged and skipped, while a dead database or an unreachable Leantime still halts
the run.
SyncDeletedCommand,deleteAll()anddelete()'s public signatures are unchanged, soapp:data-providers:sync-deleted --interval=P1Wis called exactly as before.Screenshot of the result
Not applicable — no user interface is affected.
Checklist
task test— 414 tests, 19216 assertions, green.task coding-standards:checkandmarkdownlint clean.
New and updated coverage:
Unit\Service\LeantimeApiServiceTest, alongside theupdateAsJob()ones from fix: stop the Leantime pagination cursor restarting the sync #326: the next page starts after the highest
deletionId; the cursor followsdeletionIdand not the entity id, with the two deliberately in opposite orders; adeletion with no entity id still moves the cursor; a full page with no usable
deletionIdstops and logs; a partial page queues nothing.Integration\Service\LeantimeApiServiceTest::testDeletednow feeds one flat per-typepayload per request and asserts all four request bodies, including that the timestamp
goes out under
deleted— the key the plugin actually reads. Every existing assertion iskept: the invoiced project and issue survive,
sourceDeletedDateis stamped, and therows behind the entry with an unparsable date still get processed.
Unit\MessageHandler\LeantimeDeleteHandlerTestupdated for the new argument list.As with
updateAsJob(), the integration test does not exercise the requeue: its next-pagemessage would be handled inline by the container's real service and hit the network. That
path is covered by the unit tests instead.