Skip to content

[11.x] Fix chunked queries not honoring user-defined limits and offsets #52075

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

Closed
wants to merge 14 commits into from
Closed

[11.x] Fix chunked queries not honoring user-defined limits and offsets #52075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5d7d5a4
Chunks with pre-existing limits
tonysm Jul 9, 2024
7802398
Handles pre-existing offsets on chunks
tonysm Jul 9, 2024
26dc385
Tweaks comments
tonysm Jul 9, 2024
04ee53b
Tweaks comments
tonysm Jul 9, 2024
ae785e7
Handles existing limits and offsets in the chunkById() method
tonysm Jul 9, 2024
7f6426a
Favor casting with intval() instead of ternary for offset
tonysm Jul 9, 2024
b2b82ce
Adds getter methods for offset and limit
tonysm Jul 9, 2024
2aefcfa
Remove casting as its being done in the getter methods
tonysm Jul 9, 2024
b5ad45a
StyleCI
tonysm Jul 9, 2024
0356472
Adds getter methods to the eloquent query Builder too
tonysm Jul 9, 2024
6fb5cfa
Fix mocked tests
tonysm Jul 9, 2024
c228010
StyleCI
tonysm Jul 9, 2024
e354bc9
Fix mocked tests
tonysm Jul 9, 2024
7b12ee9
Adds intval to the skip so we cast null to zero
tonysm Jul 9, 2024
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
57 changes: 51 additions & 6 deletions src/Illuminate/Database/Concerns/BuildsQueries.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,41 @@ public function chunk($count, callable $callback)
{
$this->enforceOrderBy();

// Taking into account user-defined limits and offsets (if any) for more precise control over chunks...
$skip = $this->getOffset();
$remaining = $this->getLimit();

$page = 1;

do {
// We'll execute the query for the given page and get the results. If there are
// no results we can just break and return from here. When there are results
// we will call the callback with the current chunk of these results here.
$results = $this->forPage($page, $count)->get();
// Calculating the offset while considering any existing query offset
// ensures user-defined offsets stay independent of the chunk size
// and page numbers, providing precise control over the dataset.
$offset = (($page - 1) * $count) + intval($skip);

// If a limit was defined, we'll use that as the upper bound for chunks.
// We'll decrement from the remainder limit on every iteration, until
// the limit is reached. Otherwise, we use the chunk size as limit.
$limit = is_null($remaining) ? $count : min($count, $remaining);

// Saves an unnecessary database query when the limit is already zero...
if ($limit == 0) {
break;
}

$results = $this->offset($offset)->limit($limit)->get();

$countResults = $results->count();

if ($countResults == 0) {
break;
}

// Decrements from the remainder (user-defined limits, if any) on each chunked iteration...
if (! is_null($remaining)) {
$remaining = max($remaining - $countResults, 0);
}

// On each chunk result set, we will pass them to the callback and then let the
// developer take care of everything within the callback, which allows us to
// keep the memory low for spinning through large result sets for working.
Expand Down Expand Up @@ -158,18 +179,37 @@ public function orderedChunkById($count, callable $callback, $column = null, $al

$lastId = null;

// Taking into account user-defined limits and offsets (if any) for more precise control over chunks...
$skip = $this->getOffset();
$remaining = $this->getLimit();

$page = 1;

do {
$clone = clone $this;

// Any pre-existing offset should be reset after the first page, since we'll use the $lastId from there...
if ($skip && $page > 1) {
$clone->offset(0);
}

// If a limit was defined, we'll use that as the upper bound for chunks.
// We'll decrement from the remainder limit on every iteration, until
// the limit is reached. Otherwise, we use the chunk size as limit.
$limit = is_null($remaining) ? $count : min($count, $remaining);

// Saves an unnecessary database query when the limit is already zero...
if ($limit == 0) {
break;
}

// We'll execute the query for the given page and get the results. If there are
// no results we can just break and return from here. When there are results
// we will call the callback with the current chunk of these results here.
if ($descending) {
$results = $clone->forPageBeforeId($count, $lastId, $column)->get();
$results = $clone->forPageBeforeId($limit, $lastId, $column)->get();
} else {
$results = $clone->forPageAfterId($count, $lastId, $column)->get();
$results = $clone->forPageAfterId($limit, $lastId, $column)->get();
}

$countResults = $results->count();
Expand All @@ -178,6 +218,11 @@ public function orderedChunkById($count, callable $callback, $column = null, $al
break;
}

// Decrements from the remainder (user-defined limits, if any) on each chunked iteration...
if (! is_null($remaining)) {
$remaining = max($remaining - $countResults, 0);
}

// On each chunk result set, we will pass them to the callback and then let the
// developer take care of everything within the callback, which allows us to
// keep the memory low for spinning through large result sets for working.
Expand Down
20 changes: 20 additions & 0 deletions src/Illuminate/Database/Eloquent/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,26 @@ public function cursorPaginate($perPage = null, $columns = ['*'], $cursorName =
return $this->paginateUsingCursor($perPage, $columns, $cursorName, $cursor);
}

/**
* Get the "offset" value from the query or null if it's not set.
*
* @return mixed
*/
public function getOffset()
{
return $this->query->getOffset();
}

/**
* Get the current "limit" value from the query or null if not set.
*
* @return mixed
*/
public function getLimit()
{
return $this->query->getLimit();
}

/**
* Ensure the proper order by required for cursor pagination.
*
Expand Down
24 changes: 24 additions & 0 deletions src/Illuminate/Database/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2610,6 +2610,18 @@ public function offset($value)
return $this;
}

/**
* Get the "offset" value from the query or null if it's not set.
*
* @return mixed
*/
public function getOffset()
{
$value = $this->unions ? $this->unionOffset : $this->offset;

return ! is_null($value) ? (int) $value : null;
}

/**
* Alias to set the "limit" value of the query.
*
Expand Down Expand Up @@ -2638,6 +2650,18 @@ public function limit($value)
return $this;
}

/**
* Get the current "limit" value from the query or null if not set.
*
* @return mixed
*/
public function getLimit()
{
$value = $this->unions ? $this->unionLimit : $this->limit;

return ! is_null($value) ? (int) $value : null;
}

/**
* Add a "group limit" clause to the query.
*
Expand Down
68 changes: 41 additions & 27 deletions tests/Database/DatabaseEloquentBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,15 +369,19 @@ public function testValueOrFailMethodWithModelNotFoundThrowsModelNotFoundExcepti

public function testChunkWithLastChunkComplete()
{
$builder = m::mock(Builder::class.'[forPage,get]', [$this->getMockQueryBuilder()]);
$builder = m::mock(Builder::class.'[getOffset,getLimit,offset,limit,get]', [$this->getMockQueryBuilder()]);
$builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc'];

$chunk1 = new Collection(['foo1', 'foo2']);
$chunk2 = new Collection(['foo3', 'foo4']);
$chunk3 = new Collection([]);
$builder->shouldReceive('forPage')->once()->with(1, 2)->andReturnSelf();
$builder->shouldReceive('forPage')->once()->with(2, 2)->andReturnSelf();
$builder->shouldReceive('forPage')->once()->with(3, 2)->andReturnSelf();

$builder->shouldReceive('getOffset')->once()->andReturn(null);
$builder->shouldReceive('getLimit')->once()->andReturn(null);
$builder->shouldReceive('offset')->once()->with(0)->andReturnSelf();
$builder->shouldReceive('offset')->once()->with(2)->andReturnSelf();
$builder->shouldReceive('offset')->once()->with(4)->andReturnSelf();
$builder->shouldReceive('limit')->times(3)->with(2)->andReturnSelf();
$builder->shouldReceive('get')->times(3)->andReturn($chunk1, $chunk2, $chunk3);

$callbackAssertor = m::mock(stdClass::class);
Expand All @@ -392,13 +396,16 @@ public function testChunkWithLastChunkComplete()

public function testChunkWithLastChunkPartial()
{
$builder = m::mock(Builder::class.'[forPage,get]', [$this->getMockQueryBuilder()]);
$builder = m::mock(Builder::class.'[getOffset,getLimit,offset,limit,get]', [$this->getMockQueryBuilder()]);
$builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc'];

$chunk1 = new Collection(['foo1', 'foo2']);
$chunk2 = new Collection(['foo3']);
$builder->shouldReceive('forPage')->once()->with(1, 2)->andReturnSelf();
$builder->shouldReceive('forPage')->once()->with(2, 2)->andReturnSelf();
$builder->shouldReceive('getOffset')->once()->andReturn(null);
$builder->shouldReceive('getLimit')->once()->andReturn(null);
$builder->shouldReceive('offset')->once()->with(0)->andReturnSelf();
$builder->shouldReceive('offset')->once()->with(2)->andReturnSelf();
$builder->shouldReceive('limit')->twice()->with(2)->andReturnSelf();
$builder->shouldReceive('get')->times(2)->andReturn($chunk1, $chunk2);

$callbackAssertor = m::mock(stdClass::class);
Expand All @@ -412,13 +419,16 @@ public function testChunkWithLastChunkPartial()

public function testChunkCanBeStoppedByReturningFalse()
{
$builder = m::mock(Builder::class.'[forPage,get]', [$this->getMockQueryBuilder()]);
$builder = m::mock(Builder::class.'[getOffset,getLimit,offset,limit,get]', [$this->getMockQueryBuilder()]);
$builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc'];

$chunk1 = new Collection(['foo1', 'foo2']);
$chunk2 = new Collection(['foo3']);
$builder->shouldReceive('forPage')->once()->with(1, 2)->andReturnSelf();
$builder->shouldReceive('forPage')->never()->with(2, 2);

$builder->shouldReceive('getOffset')->once()->andReturn(null);
$builder->shouldReceive('getLimit')->once()->andReturn(null);
$builder->shouldReceive('offset')->once()->with(0)->andReturnSelf();
$builder->shouldReceive('limit')->once()->with(2)->andReturnSelf();
$builder->shouldReceive('get')->times(1)->andReturn($chunk1);

$callbackAssertor = m::mock(stdClass::class);
Expand All @@ -434,29 +444,30 @@ public function testChunkCanBeStoppedByReturningFalse()

public function testChunkWithCountZero()
{
$builder = m::mock(Builder::class.'[forPage,get]', [$this->getMockQueryBuilder()]);
$builder = m::mock(Builder::class.'[getOffset,getLimit,offset,limit,get]', [$this->getMockQueryBuilder()]);
$builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc'];

$chunk = new Collection([]);
$builder->shouldReceive('forPage')->once()->with(1, 0)->andReturnSelf();
$builder->shouldReceive('get')->times(1)->andReturn($chunk);

$callbackAssertor = m::mock(stdClass::class);
$callbackAssertor->shouldReceive('doSomething')->never();
$builder->shouldReceive('getOffset')->once()->andReturn(null);
$builder->shouldReceive('getLimit')->once()->andReturn(null);
$builder->shouldReceive('offset')->never();
$builder->shouldReceive('limit')->never();
$builder->shouldReceive('get')->never();

$builder->chunk(0, function ($results) use ($callbackAssertor) {
$callbackAssertor->doSomething($results);
$builder->chunk(0, function () {
$this->fail('Should not be called.');
});
}

public function testChunkPaginatesUsingIdWithLastChunkComplete()
{
$builder = m::mock(Builder::class.'[forPageAfterId,get]', [$this->getMockQueryBuilder()]);
$builder = m::mock(Builder::class.'[getOffset,getLimit,forPageAfterId,get]', [$this->getMockQueryBuilder()]);
$builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc'];

$chunk1 = new Collection([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]);
$chunk2 = new Collection([(object) ['someIdField' => 10], (object) ['someIdField' => 11]]);
$chunk3 = new Collection([]);
$builder->shouldReceive('getOffset')->andReturnNull();
$builder->shouldReceive('getLimit')->andReturnNull();
$builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturnSelf();
$builder->shouldReceive('forPageAfterId')->once()->with(2, 2, 'someIdField')->andReturnSelf();
$builder->shouldReceive('forPageAfterId')->once()->with(2, 11, 'someIdField')->andReturnSelf();
Expand All @@ -474,11 +485,13 @@ public function testChunkPaginatesUsingIdWithLastChunkComplete()

public function testChunkPaginatesUsingIdWithLastChunkPartial()
{
$builder = m::mock(Builder::class.'[forPageAfterId,get]', [$this->getMockQueryBuilder()]);
$builder = m::mock(Builder::class.'[getOffset,getLimit,forPageAfterId,get]', [$this->getMockQueryBuilder()]);
$builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc'];

$chunk1 = new Collection([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]);
$chunk2 = new Collection([(object) ['someIdField' => 10]]);
$builder->shouldReceive('getOffset')->andReturnNull();
$builder->shouldReceive('getLimit')->andReturnNull();
$builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturnSelf();
$builder->shouldReceive('forPageAfterId')->once()->with(2, 2, 'someIdField')->andReturnSelf();
$builder->shouldReceive('get')->times(2)->andReturn($chunk1, $chunk2);
Expand All @@ -494,18 +507,19 @@ public function testChunkPaginatesUsingIdWithLastChunkPartial()

public function testChunkPaginatesUsingIdWithCountZero()
{
$builder = m::mock(Builder::class.'[forPageAfterId,get]', [$this->getMockQueryBuilder()]);
$builder = m::mock(Builder::class.'[getOffset,getLimit,forPageAfterId,get]', [$this->getMockQueryBuilder()]);
$builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc'];

$chunk = new Collection([]);
$builder->shouldReceive('forPageAfterId')->once()->with(0, 0, 'someIdField')->andReturnSelf();
$builder->shouldReceive('get')->times(1)->andReturn($chunk);
$builder->shouldReceive('getOffset')->andReturnNull();
$builder->shouldReceive('getLimit')->andReturnNull();
$builder->shouldReceive('forPageAfterId')->never();
$builder->shouldReceive('get')->never();

$callbackAssertor = m::mock(stdClass::class);
$callbackAssertor->shouldReceive('doSomething')->never();

$builder->chunkById(0, function ($results) use ($callbackAssertor) {
$callbackAssertor->doSomething($results);
$builder->chunkById(0, function () {
$this->fail('Should never be called.');
}, 'someIdField');
}

Expand Down
Loading