Skip to content

Fix: Paginator Iterator fails on single pages and skips last page in multi-page results #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jonathanpmartins wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix: Paginator Iterator fails on single pages and skips last page in multi-page results #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb58343
sometimes the API error returns a description instead of a message
jonathanpmartins Jun 13, 2025
9222c93
Last Page Bug Fix + Tests
jonathanpmartins Sep 18, 2025
f9251c6
make all phpstan stuff pass
jonathanpmartins Sep 18, 2025
c3930b7
removing @phpstan-ignore-next-line
jonathanpmartins Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Paginator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
* $result = $paginator->current();
* ```
*
* @implements Iterator<int, array<mixed>>
* @implements Iterator<int, array<array>>
* @phpstan-type Pagination array{skip: int, limit: int, totalCount: int, hasPreviousPage: bool, hasNextPage: bool}
*/
class Paginator implements Iterator
Expand Down Expand Up @@ -180,7 +180,7 @@ public function valid(): bool
return true;
}

return $this->getPagination()["hasNextPage"];
return $this->skip < $this->getPagination()["totalCount"];
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/RequestTransport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ private function hydrateResponse(ResponseInterface $response): array
$error = $contents["error"];

if (is_array($error)) {
$error = $error["message"];
$error = $error["message"] ?? $error["description"];
}

throw new ApiErrorException($error);
Expand Down
276 changes: 274 additions & 2 deletions tests/PaginatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,24 @@ public function testCurrent(): void

public function testValid(): void
{
$paginator = $this->makePaginator(["pageInfo" => ["hasNextPage" => false]]);
$paginator = $this->makePaginator();
$this->assertTrue($paginator->valid(), 'Should be valid when lastResult is null');

$singlePageResult = [
"pageInfo" => [
"skip" => 0,
"limit" => 30,
"totalCount" => 7,
"hasNextPage" => false,
"hasPreviousPage" => false
]
];
$paginator = $this->makePaginator($singlePageResult);
$this->assertTrue($paginator->valid(), 'Should be valid for single page with data');

$this->assertFalse($paginator->valid());
$paginator = $this->makePaginator($singlePageResult);
$paginator->skip(10);
$this->assertFalse($paginator->valid(), 'Should be invalid when skip > totalCount');
}

public function testGetTotalResourcesCount(): void
Expand All @@ -99,6 +114,263 @@ public function testGetTotalResourcesCount(): void
$this->assertSame(10, $paginator->getTotalResourcesCount());
}

public function testSinglePageIteration(): void
{
$singlePageResponse = [
'transactions' => [
['id' => 1, 'amount' => 100],
['id' => 2, 'amount' => 200],
['id' => 3, 'amount' => 300],
['id' => 4, 'amount' => 400],
['id' => 5, 'amount' => 500],
['id' => 6, 'amount' => 600],
['id' => 7, 'amount' => 700],
],
'pageInfo' => [
'skip' => 0,
'limit' => 30,
'totalCount' => 7,
'hasPreviousPage' => false,
'hasNextPage' => false,
]
];

$listRequestMock = $this->createMock(Request::class);
$listRequestMock->expects($this->once())
->method("pagination")
->willReturnSelf();

$requestTransportMock = $this->createMock(RequestTransport::class);
$requestTransportMock->expects($this->once())
->method('transport')
->with($listRequestMock)
->willReturn($singlePageResponse);

$paginator = new Paginator($requestTransportMock, $listRequestMock);

$iterations = 0;
$processedTransactions = [];

foreach ($paginator as $result)
{
$iterations++;
$this->assertArrayHasKey('transactions', $result);
$this->assertArrayHasKey('pageInfo', $result);
$this->assertCount(7, $result['transactions']);

foreach ($result['transactions'] as $transaction)
{
$processedTransactions[] = $transaction;
}
}

$this->assertSame(1, $iterations, 'Should iterate exactly once for single page');
$this->assertCount(7, $processedTransactions, 'Should process all 7 transactions');
$this->assertSame(100, $processedTransactions[0]['amount'], 'First transaction should be accessible');
$this->assertSame(700, $processedTransactions[6]['amount'], 'Last transaction should be accessible');
}

public function testMultiPageIncludesLastPage(): void
{
$responses = [
[
'charges' => [
['id' => 1, 'amount' => 100],
['id' => 2, 'amount' => 200],
],
'pageInfo' => [
'skip' => 0,
'limit' => 2,
'totalCount' => 5,
'hasPreviousPage' => false,
'hasNextPage' => true
]
],
[
'charges' => [
['id' => 3, 'amount' => 300],
['id' => 4, 'amount' => 400],
],
'pageInfo' => [
'skip' => 2,
'limit' => 2,
'totalCount' => 5,
'hasPreviousPage' => true,
'hasNextPage' => true
]
],
[
'charges' => [
['id' => 5, 'amount' => 500],
],
'pageInfo' => [
'skip' => 4,
'limit' => 2,
'totalCount' => 5,
'hasPreviousPage' => true,
'hasNextPage' => false // Last page
]
]
];

$listRequestMock = $this->createMock(Request::class);
$listRequestMock->expects($this->exactly(3))
->method("pagination")
->willReturnSelf();

$requestTransportMock = $this->createMock(RequestTransport::class);
$requestTransportMock->expects($this->exactly(3))
->method('transport')
->with($listRequestMock)
->willReturnOnConsecutiveCalls(...$responses);

$paginator = new Paginator($requestTransportMock, $listRequestMock);
$paginator->perPage(2); // Set page size to 2 to create 3 pages

$iterations = 0;
$allCharges = [];

foreach ($paginator as $result)
{
$iterations++;
$this->assertArrayHasKey('charges', $result);

foreach ($result['charges'] as $charge)
{
$allCharges[] = $charge;
}
}

$this->assertSame(3, $iterations, 'Should iterate through all 3 pages');
$this->assertCount(5, $allCharges, 'Should process all 5 charges including last page');

$lastCharge = end($allCharges);
$this->assertSame(5, $lastCharge['id'], 'Should include charge from last page');
$this->assertSame(500, $lastCharge['amount'], 'Last page data should be accessible');
}

public function testEmptyResultsIteration(): void
{
$emptyResponse = [
'items' => [],
'pageInfo' => [
'skip' => 0,
'limit' => 30,
'totalCount' => 0,
'hasPreviousPage' => false,
'hasNextPage' => false
]
];

$listRequestMock = $this->createMock(Request::class);
$listRequestMock->expects($this->once())
->method("pagination")
->willReturnSelf();

$requestTransportMock = $this->createMock(RequestTransport::class);
$requestTransportMock->expects($this->once())
->method('transport')
->with($listRequestMock)
->willReturn($emptyResponse);

$paginator = new Paginator($requestTransportMock, $listRequestMock);

$iterations = 0;
foreach ($paginator as $result)
{
$iterations++;
$this->assertArrayHasKey('items', $result);
$this->assertEmpty($result['items']);
}

$this->assertSame(1, $iterations, 'Should iterate once even for empty results');
}

public function testOriginalBugReportScenario(): void
{
$getTransactionsResponse = [
'transactions' => [
['id' => 't1', 'amount' => 1000, 'date' => '2024-01-01'],
['id' => 't2', 'amount' => 2000, 'date' => '2024-01-02'],
['id' => 't3', 'amount' => 3000, 'date' => '2024-01-03'],
['id' => 't4', 'amount' => 4000, 'date' => '2024-01-04'],
['id' => 't5', 'amount' => 5000, 'date' => '2024-01-05'],
['id' => 't6', 'amount' => 6000, 'date' => '2024-01-06'],
['id' => 't7', 'amount' => 7000, 'date' => '2024-01-07'],
],
'pageInfo' => [
'skip' => 0,
'limit' => 30,
'totalCount' => 7,
'hasPreviousPage' => false,
'hasNextPage' => false
]
];

$listRequestMock = $this->createMock(Request::class);
$listRequestMock->expects($this->once())
->method("pagination")
->willReturnSelf();

$requestTransportMock = $this->createMock(RequestTransport::class);
$requestTransportMock->expects($this->once())
->method('transport')
->with($listRequestMock)
->willReturn($getTransactionsResponse);

$paginator = new Paginator($requestTransportMock, $listRequestMock);

$outerLoopExecuted = false;
$transactionIds = [];

foreach ($paginator as $result)
{
$outerLoopExecuted = true;

foreach ($result['transactions'] as $transaction)
{
$transactionIds[] = $transaction['id'];
}
}

$this->assertTrue($outerLoopExecuted, 'Outer foreach loop must execute');
$this->assertCount(7, $transactionIds, 'Should access all 7 transactions');
$this->assertSame(['t1', 't2', 't3', 't4', 't5', 't6', 't7'], $transactionIds);
}

public function testValidWithDifferentSkipPositions(): void
{
$result = [
"pageInfo" => [
"skip" => 0,
"limit" => 10,
"totalCount" => 25,
"hasNextPage" => true,
"hasPreviousPage" => false
]
];

$paginator = $this->makePaginator($result);
$paginator->skip(0);
$this->assertTrue($paginator->valid(), 'Should be valid at position 0');

$paginator = $this->makePaginator($result);
$paginator->skip(15);
$this->assertTrue($paginator->valid(), 'Should be valid when skip < totalCount');

$paginator = $this->makePaginator($result);
$paginator->skip(24);
$this->assertTrue($paginator->valid(), 'Should be valid when skip = totalCount - 1');

$paginator = $this->makePaginator($result);
$paginator->skip(25);
$this->assertFalse($paginator->valid(), 'Should be invalid when skip >= totalCount');

$paginator = $this->makePaginator($result);
$paginator->skip(30);
$this->assertFalse($paginator->valid(), 'Should be invalid when skip > totalCount');
}

private function testPaginatorNavigation(callable $navigate, int $expectedSkip = 0, int $skip = 0, int $perPage = 30): void
{
$paginator = $this->makePaginator();
Expand Down