Fix pagination for all permutations of first, last, before and after - #214
Merged
TomHAnderson merged 2 commits intoSep 14, 2026
Merged
Conversation
Pagination arguments were normalized to int with 0 as the default, then
tested for truthiness, so a cursor for index zero was indistinguishable
from an absent argument. Combined with an if/elseif on the offset, this
silently discarded arguments and, in two cases, returned the wrong end of
the result set.
PaginationService now resolves the arguments by narrowing a range over the
counted result set, which is the algorithm the Complete Connection Model
describes. All three resolvers share it and agree on every permutation.
Fixed:
- before at index 0 was read as absent, so { before: <first cursor> }
returned a full page and { last: n, before: <first cursor> } returned the
last n rows instead of nothing
- after + before discarded before; last + after discarded after
- last greater than the row count threw "Offset must be a positive integer
or zero" from Doctrine on the ORM paths, which DBAL avoided only because
it clamped separately
- an empty page reported hasNextPage true and hasPreviousPage false, so a
conforming client looped forever
- first: 0 returned a full page rather than no rows
- negative counts and undecodable cursors were coerced and silently applied
- first + before returned the last n rows before the cursor, not the first
- first + last produced a plausible but meaningless page
The QueryBuilder event must be dispatched before the count so a listener
can modify the QueryBuilder, and a backward request cannot know its offset
until the rows are counted, so getOffset and getLimit are renamed to
getRequestedOffset and getRequestedLimit to say what they return. A
listener following the docs and fetching a large dataset itself previously
read the first page on a last query.
Breaking changes for 13.2, with migration notes in docs/upgrade.rst:
- PageInfo.startCursor and PageInfo.endCursor are nullable String rather
than String!, and are null when edges is empty
- Event\QueryBuilder::getOffset/getLimit renamed as above
- PaginationService::decodePaginationFields returns int|null per field and
throws Exception\Pagination on invalid input
- PaginationService::calculateOffsetAndLimit requires int $itemCount
- PaginationService::buildCursors takes (int $offset, int $resultCount) and
returns start and end keys only
- PaginationService::buildPaginationResponse takes int $offset
- Adds PaginationService::calculateRequestedOffsetAndLimit and
Exception\Pagination
PaginationPermutationTest runs the full matrix against the DBAL and entity
resolvers plus the association cases against the collection resolver, so
the three stay in agreement.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 13.2.x #214 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 638 646 +8
===========================================
Files 60 61 +1
Lines 1926 1963 +37
===========================================
+ Hits 1926 1963 +37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pagination arguments were normalized to int with 0 as the default, then tested for truthiness, so a cursor for index zero was indistinguishable from an absent argument. Combined with an if/elseif on the offset, this silently discarded arguments and, in two cases, returned the wrong end of the result set.
PaginationService now resolves the arguments by narrowing a range over the counted result set, which is the algorithm the Complete Connection Model describes. All three resolvers share it and agree on every permutation.
Fixed:
The QueryBuilder event must be dispatched before the count so a listener can modify the QueryBuilder, and a backward request cannot know its offset until the rows are counted, so getOffset and getLimit are renamed to getRequestedOffset and getRequestedLimit to say what they return. A listener following the docs and fetching a large dataset itself previously read the first page on a last query.
Breaking changes for 13.2, with migration notes in docs/upgrade.rst:
PaginationPermutationTest runs the full matrix against the DBAL and entity resolvers plus the association cases against the collection resolver, so the three stay in agreement.