Skip to content

feat: paginated the deleted endpoint and narrowed it to one type - #21

Merged
tuj merged 3 commits into
developfrom
feature/8000-deleted-pagination
Aug 20, 2026
Merged

feat: paginated the deleted endpoint and narrowed it to one type#21
tuj merged 3 commits into
developfrom
feature/8000-deleted-pagination

Conversation

@tuj

@tuj tuj commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Link to ticket

https://leantime.itkdev.dk/#/tickets/showTicket/8000

Description

/apidata/api/deleted was the only endpoint with no bound on its result set —
ApiDataRepository::getDeleted() was the only query in the class with no orderBy, no
limit and no offset, so every request returned each named type's entire deletion
history in unspecified order. types was required precisely because of that.

It now serves one type per request, paginated, in the shape the entity endpoints
already use:

curl https://leantime.local.itkdev.dk/apidata/api/deleted \
  -H "x-api-key: lt_1234567890" -H "Content-Type: application/json" \
  -d '{"type":"tickets","start":0,"limit":100,"deleted":1759906882}'
  {
    "parameters": {"type": "tickets", "start": 0, "limit": 100, "deleted": 1759906882},
    "resultsCount": 2,
    "results": [
      {"deletionId": 82, "id": 4711, "deletedDate": "2026-03-04T08:15:00+00:00"},
      {"deletionId": 84, "id": 4712, "deletedDate": "2026-03-04T09:20:00+00:00"}
    ]
  }

Three decisions worth reviewing:

The cursor is the tracking table's own id, exposed as deletionId. Deletions are
appended as they happen, so that is the one column that orders them and does not move
under a paging client — entryId is whatever was deleted, in no particular order, and
paging on it would skip deletions. No schema change was needed: id int NOT NULL AUTO_INCREMENT PRIMARY KEY is already in the frozen baseline for all three
itk_*_deleted tables, so the cursor is the primary key index on every existing install.

type is singular and required, and a list is a 400 (type must be a single value.),
rather than being reduced to its first element — a caller still sending the old types
should be told, not quietly served a page of a type it did not ask about. An unknown type
still answers 400 without echoing the input back onto the error page.

results is a flat list, like every other endpoint, which is what makes
resultsCount === limit a meaningful "request the next page" signal. resultsCount was
previously the sum across all types, which meant nothing to a paging client.

DEFAULT_LIMIT, MAX_LIMIT and toLimit() moved from RequestParameters into the
CoercesRequestInput trait, so both parameter objects share one implementation of the
"at least 1, silently capped at 1000, echoed in parameters" rule instead of duplicating
it.

Breaking change. types → type and the flat results both break existing callers.
The consumer side is
economics#333 (itk-dev/economics#333), which pages per type and
follows deletionId; the two have to ship together.

Screenshot of the result

Not applicable — no user interface is affected.

Checklist

  • My code is covered by test cases.
  • My code passes our test (all our tests).
  • My code passes our static analysis suite.
  • My code passes our continuous integration process.

task test — 75 tests, 373 assertions, green. task lint clean.

New and updated coverage:

  • DeletedRequestParametersTest — rewritten for type: missing, empty, a list, an unknown
    value and users are all rejected; start/limit coercion mirrors
    RequestParametersTest, including the cap and a limit below 1.
  • APIDataTest::testGetDeletedKeepsTheDeletionIdApartFromTheDeletedEntityId — both ids are
    plain ints, so nothing but an explicit assertion catches them being swapped, and a
    client paging on a swapped deletionId would walk entity ids and silently skip
    deletions.
  • DeletedDataTest — the new field, nullable alongside the existing ones.

Controllers/ is still outside phpunit.xml.dist's , so API::getDeleted()
itself remains untested; testing it needs a stub for Leantime's Controller base class,
which this PR does not add.

Additional comments or questions

The old response shape let a caller fetch several types in one round trip. That is now
one request per type — four instead of one for a full delete sync. Each is bounded and
indexed, so the trade seemed clearly worth it, but say so if you'd rather keep a
multi-type form alongside the paged one.

The endpoint returned every type's whole deletion history in one unbounded
response: it was the only query in ApiDataRepository with no orderBy, no limit
and no offset, and `types` was required precisely because of that.

It now serves one type per request, paged with start/limit like the entity
endpoints. The cursor is the tracking table's own auto-increment id, exposed as
`deletionId` next to the deleted entity's `id` — deletions are appended, so that
is the one column that orders them and stays put while a client pages through.
`results` becomes a flat list, which makes resultsCount === limit a meaningful
"there may be more" signal.
@tuj tuj self-assigned this Aug 19, 2026
It filters on a timestamp the same way `modifiedAfter` does on the entity
endpoints, so it now carries the same name.

The old `deleted` answers 400 instead of being ignored. Ignored, it would answer
with the whole deletion history while the caller believes it asked for a window —
which is exactly what happened to the consumer when it sent the timestamp under
the name the endpoint did not read.
@tuj
tuj requested a review from turegjorup August 19, 2026 08:28

@turegjorup turegjorup left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with comments, 1) should be fixed, the other two are edge cases I don't think we need to account for.

  1. README.md:86 — medium. The paging instruction says to keep requesting pages "for as long as resultsCount equals the limit you asked for", but toLimit() silently caps at 1000. A client asking for limit=5000 gets 1000 back, resultsCount never equals its requested limit, so it stops after one page and treats the delete sync as complete — silently missing everything past the first 1000. State the termination condition against the applied limit (parameters.limit in the response).

  2. ApiDataRepository.php:117 — low. The entry.id >= $startId watermark assumes auto-increment ids commit in id order. Under InnoDB's default innodb_autoinc_lock_mode=2 a transaction holding id 100 can commit after one holding 101; a client polling in that window sees only 101, advances its watermark past 100, and never sees that deletion again. The entity endpoints have modifiedAfter as a safety net — this one has none if the consumer follows deletionId alone, which the PR body says economics#333 does. Either note it in the README or have the consumer re-scan with a deletedAfter overlap window.

  3. DeletedRequestParameters.php:91 — low. The comment on the is_array() branch says a list is rejected because a caller still sending the old types would otherwise get a type it didn't ask for — but that caller sends the key types, which never reaches toType(); it falls through to the generic "type is required" message. The branch only fires on type[]=. No wrong data served, but the migration hint you deliberately give for deleted→deletedAfter via rejectTheOldDeletedName() is missing for types→type, and the comment describes a case the code doesn't cover. Either add a matching guard or fix the comment.

@tuj
tuj merged commit c0163cb into develop Aug 20, 2026
3 checks passed
@tuj
tuj deleted the feature/8000-deleted-pagination branch August 20, 2026 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants