From 9608f7eb10ed9c07558447fed2bc1ffcca3f4625 Mon Sep 17 00:00:00 2001 From: vlakoff Date: Tue, 8 Nov 2016 14:08:55 +0100 Subject: [PATCH 1/2] Enforce an orderBy clause for chunk() and chunkById() --- src/Illuminate/Database/Eloquent/Builder.php | 20 +++++++++++++--- src/Illuminate/Database/Query/Builder.php | 24 +++++++++++++++---- .../Database/DatabaseEloquentBuilderTest.php | 10 ++++++++ tests/Database/DatabaseQueryBuilderTest.php | 12 ++++++++++ 4 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Builder.php b/src/Illuminate/Database/Eloquent/Builder.php index d1fb30f6418c..4b2ae4c3ca08 100755 --- a/src/Illuminate/Database/Eloquent/Builder.php +++ b/src/Illuminate/Database/Eloquent/Builder.php @@ -394,6 +394,8 @@ public function cursor() */ public function chunk($count, callable $callback) { + $this->enforceOrderBy(); + $page = 1; do { @@ -428,6 +430,8 @@ public function chunk($count, callable $callback) */ public function chunkById($count, callable $callback, $column = 'id') { + $this->enforceOrderBy(); + $lastId = 0; do { @@ -458,9 +462,7 @@ public function chunkById($count, callable $callback, $column = 'id') */ public function each(callable $callback, $count = 1000) { - if (empty($this->query->orders) && empty($this->query->unionOrders)) { - $this->orderBy($this->model->getQualifiedKeyName(), 'asc'); - } + $this->enforceOrderBy(); return $this->chunk($count, function ($results) use ($callback) { foreach ($results as $key => $value) { @@ -471,6 +473,18 @@ public function each(callable $callback, $count = 1000) }); } + /** + * Add a generic orderBy clause if the query doesn't already have one. + * + * @return void + */ + protected function enforceOrderBy() + { + if (empty($this->query->orders) && empty($this->query->unionOrders)) { + $this->orderBy($this->model->getQualifiedKeyName(), 'asc'); + } + } + /** * Get an array with the values of a given column. * diff --git a/src/Illuminate/Database/Query/Builder.php b/src/Illuminate/Database/Query/Builder.php index 25ba419c73bd..3940689c161e 100755 --- a/src/Illuminate/Database/Query/Builder.php +++ b/src/Illuminate/Database/Query/Builder.php @@ -1807,6 +1807,8 @@ public function cursor() */ public function chunk($count, callable $callback) { + $this->enforceOrderBy(); + $page = 1; do { @@ -1842,6 +1844,8 @@ public function chunk($count, callable $callback) */ public function chunkById($count, callable $callback, $column = 'id', $alias = null) { + $this->enforceOrderBy(); + $alias = $alias ?: $column; $lastId = 0; @@ -1871,14 +1875,10 @@ public function chunkById($count, callable $callback, $column = 'id', $alias = n * @param callable $callback * @param int $count * @return bool - * - * @throws \RuntimeException */ public function each(callable $callback, $count = 1000) { - if (empty($this->orders) && empty($this->unionOrders)) { - throw new RuntimeException('You must specify an orderBy clause when using the "each" function.'); - } + $this->enforceOrderBy(); return $this->chunk($count, function ($results) use ($callback) { foreach ($results as $key => $value) { @@ -1889,6 +1889,20 @@ public function each(callable $callback, $count = 1000) }); } + /** + * Throw an exception if the query doesn't have an orderBy clause. + * + * @return void + * + * @throws \RuntimeException + */ + protected function enforceOrderBy() + { + if (empty($this->orders) && empty($this->unionOrders)) { + throw new RuntimeException('You must specify an orderBy clause when using this function.'); + } + } + /** * Get an array with the values of a given column. * diff --git a/tests/Database/DatabaseEloquentBuilderTest.php b/tests/Database/DatabaseEloquentBuilderTest.php index bd6722fb9c6e..cd33a2278800 100755 --- a/tests/Database/DatabaseEloquentBuilderTest.php +++ b/tests/Database/DatabaseEloquentBuilderTest.php @@ -157,6 +157,8 @@ public function testValueMethodWithModelNotFound() public function testChunkWithLastChunkComplete() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,get]', [$this->getMockQueryBuilder()]); + $builder->getQuery()->orders[] = ['column' => 'foobar', 'direction' => 'asc']; + $chunk1 = new Collection(['foo1', 'foo2']); $chunk2 = new Collection(['foo3', 'foo4']); $chunk3 = new Collection([]); @@ -178,6 +180,8 @@ public function testChunkWithLastChunkComplete() public function testChunkWithLastChunkPartial() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,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(); @@ -196,6 +200,8 @@ public function testChunkWithLastChunkPartial() public function testChunkCanBeStoppedByReturningFalse() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[forPage,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(); @@ -216,6 +222,8 @@ public function testChunkCanBeStoppedByReturningFalse() public function testChunkPaginatesUsingIdWithLastChunkComplete() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[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([]); @@ -237,6 +245,8 @@ public function testChunkPaginatesUsingIdWithLastChunkComplete() public function testChunkPaginatesUsingIdWithLastChunkPartial() { $builder = m::mock('Illuminate\Database\Eloquent\Builder[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('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturnSelf(); diff --git a/tests/Database/DatabaseQueryBuilderTest.php b/tests/Database/DatabaseQueryBuilderTest.php index fff1d775f035..184fc991e19e 100755 --- a/tests/Database/DatabaseQueryBuilderTest.php +++ b/tests/Database/DatabaseQueryBuilderTest.php @@ -1743,6 +1743,8 @@ public function testTableValuedFunctionAsTableInSqlServer() public function testChunkWithLastChunkComplete() { $builder = $this->getMockQueryBuilder(); + $builder->orders[] = ['column' => 'foobar', 'direction' => 'asc']; + $chunk1 = collect(['foo1', 'foo2']); $chunk2 = collect(['foo3', 'foo4']); $chunk3 = collect([]); @@ -1764,6 +1766,8 @@ public function testChunkWithLastChunkComplete() public function testChunkWithLastChunkPartial() { $builder = $this->getMockQueryBuilder(); + $builder->orders[] = ['column' => 'foobar', 'direction' => 'asc']; + $chunk1 = collect(['foo1', 'foo2']); $chunk2 = collect(['foo3']); $builder->shouldReceive('forPage')->once()->with(1, 2)->andReturnSelf(); @@ -1782,6 +1786,8 @@ public function testChunkWithLastChunkPartial() public function testChunkCanBeStoppedByReturningFalse() { $builder = $this->getMockQueryBuilder(); + $builder->orders[] = ['column' => 'foobar', 'direction' => 'asc']; + $chunk1 = collect(['foo1', 'foo2']); $chunk2 = collect(['foo3']); $builder->shouldReceive('forPage')->once()->with(1, 2)->andReturnSelf(); @@ -1802,6 +1808,8 @@ public function testChunkCanBeStoppedByReturningFalse() public function testChunkPaginatesUsingIdWithLastChunkComplete() { $builder = $this->getMockQueryBuilder(); + $builder->orders[] = ['column' => 'foobar', 'direction' => 'asc']; + $chunk1 = collect([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]); $chunk2 = collect([(object) ['someIdField' => 10], (object) ['someIdField' => 11]]); $chunk3 = collect([]); @@ -1823,6 +1831,8 @@ public function testChunkPaginatesUsingIdWithLastChunkComplete() public function testChunkPaginatesUsingIdWithLastChunkPartial() { $builder = $this->getMockQueryBuilder(); + $builder->orders[] = ['column' => 'foobar', 'direction' => 'asc']; + $chunk1 = collect([(object) ['someIdField' => 1], (object) ['someIdField' => 2]]); $chunk2 = collect([(object) ['someIdField' => 10]]); $builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'someIdField')->andReturnSelf(); @@ -1841,6 +1851,8 @@ public function testChunkPaginatesUsingIdWithLastChunkPartial() public function testChunkPaginatesUsingIdWithAlias() { $builder = $this->getMockQueryBuilder(); + $builder->orders[] = ['column' => 'foobar', 'direction' => 'asc']; + $chunk1 = collect([(object) ['table_id' => 1], (object) ['table_id' => 10]]); $chunk2 = collect([]); $builder->shouldReceive('forPageAfterId')->once()->with(2, 0, 'table.id')->andReturnSelf(); From 8eb7f55cc1f3ed117bc787db663b5197ea8251ea Mon Sep 17 00:00:00 2001 From: vlakoff Date: Tue, 8 Nov 2016 14:09:15 +0100 Subject: [PATCH 2/2] Redundant, already called in the subsequent chunk() --- src/Illuminate/Database/Eloquent/Builder.php | 2 -- src/Illuminate/Database/Query/Builder.php | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Builder.php b/src/Illuminate/Database/Eloquent/Builder.php index 4b2ae4c3ca08..d4bfa84a7704 100755 --- a/src/Illuminate/Database/Eloquent/Builder.php +++ b/src/Illuminate/Database/Eloquent/Builder.php @@ -462,8 +462,6 @@ public function chunkById($count, callable $callback, $column = 'id') */ public function each(callable $callback, $count = 1000) { - $this->enforceOrderBy(); - return $this->chunk($count, function ($results) use ($callback) { foreach ($results as $key => $value) { if ($callback($value, $key) === false) { diff --git a/src/Illuminate/Database/Query/Builder.php b/src/Illuminate/Database/Query/Builder.php index 3940689c161e..96db7be4ab67 100755 --- a/src/Illuminate/Database/Query/Builder.php +++ b/src/Illuminate/Database/Query/Builder.php @@ -1878,8 +1878,6 @@ public function chunkById($count, callable $callback, $column = 'id', $alias = n */ public function each(callable $callback, $count = 1000) { - $this->enforceOrderBy(); - return $this->chunk($count, function ($results) use ($callback) { foreach ($results as $key => $value) { if ($callback($value, $key) === false) {