From 1a00d45f8ba091705ae67d6fed36a8d52ce6bc15 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 01:56:46 +0700 Subject: [PATCH 01/13] Fix Model::first() only use orderBy() when group by is not empty --- system/Model.php | 4 ++-- tests/system/Database/Live/ModelTest.php | 25 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/system/Model.php b/system/Model.php index 4fe4eef2ef0f..a7f9607e5c89 100644 --- a/system/Model.php +++ b/system/Model.php @@ -472,8 +472,8 @@ public function first() } // Some databases, like PostgreSQL, need order - // information to consistently return correct results. - if (empty($builder->QBOrderBy) && ! empty($this->primaryKey)) + // information to consistently return correct results when there is group by + if (! empty($this->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) { $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); } diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index c506aa420c7e..b47a116a0ba7 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -251,6 +251,31 @@ public function testFirst() //-------------------------------------------------------------------- + public function testFirstWithoutGroupBy() + { + $model = new UserModel(); + + $user = $model->select('SUM(id) as total') + ->where('id >', 2) + ->first(); + $this->assertEquals(7, $user->total); + } + + //-------------------------------------------------------------------- + + public function testFirstWithGroupBy() + { + $model = new UserModel(); + + $user = $model->select('SUM(id) as total') + ->where('id >', 2) + ->groupBy('id') + ->first(); + $this->assertEquals(3, $user->total); + } + + //-------------------------------------------------------------------- + public function testFirstRespectsSoftDeletes() { $this->db->table('user') From 5a06e0e672f4ff24167ba4704e80828fec567098 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 02:43:18 +0700 Subject: [PATCH 02/13] handle soft delete enabeld first, force order by --- system/Model.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/system/Model.php b/system/Model.php index a7f9607e5c89..e02bbe68cd3d 100644 --- a/system/Model.php +++ b/system/Model.php @@ -473,9 +473,13 @@ public function first() // Some databases, like PostgreSQL, need order // information to consistently return correct results when there is group by - if (! empty($this->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) + // except when soft delete is enabled + if (empty($builder->QBOrderBy) && ! empty($this->primaryKey)) { - $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); + if (! empty($this->QBGroupBy) || $this->tempUseSoftDeletes === true) + { + $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); + } } $row = $builder->limit(1, 0) From 31ce7dea8130e86e6bebdc56d4714aa5fe558304 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 03:09:42 +0700 Subject: [PATCH 03/13] tests for withdeleted on group by --- tests/system/Database/Live/ModelTest.php | 56 +++++++++++++++++++++--- 1 file changed, 49 insertions(+), 7 deletions(-) diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index b47a116a0ba7..9f65cfd1e95c 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -276,23 +276,47 @@ public function testFirstWithGroupBy() //-------------------------------------------------------------------- - public function testFirstRespectsSoftDeletes() + public function provideGroupBy() + { + return [ + [true], + [false], + ]; + } + + /** + * @dataProvider provideGroupBy + */ + public function testFirstRespectsSoftDeletes($groupBy) { $this->db->table('user') ->where('id', 1) ->update(['deleted_at' => date('Y-m-d H:i:s')]); $model = new UserModel(); - - $user = $model->first(); + if ($groupBy) + { + $user = $model->groupBy('id')->first(); + } + else + { + $user = $model->first(); + } // fix for PHP7.2 $count = is_array($user) ? count($user) : 1; $this->assertEquals(1, $count); $this->assertEquals(2, $user->id); - $user = $model->withDeleted() - ->first(); + $user = $model->withDeleted(); + if ($groupBy) + { + $user = $model->groupBy('id')->first(); + } + else + { + $user = $model->first(); + } $this->assertEquals(1, $user->id); } @@ -1900,13 +1924,31 @@ public function testUndefinedMethodInBuilder() ->getBindings(); } - public function testFirstRecoverTempUseSoftDeletes() + /** + * @dataProvider provideGroupBy + */ + public function testFirstRecoverTempUseSoftDeletes($groupBy) { $model = new UserModel($this->db); $model->delete(1); - $user = $model->withDeleted()->first(); + if ($groupBy) + { + $user = $model->groupBy('id')->withDeleted()->first(); + } + else + { + $user = $model->withDeleted()->first(); + } $this->assertEquals(1, $user->id); $user2 = $model->first(); + if ($groupBy) + { + $user2 = $model->groupBy('id')->first(); + } + else + { + $user2 = $model->first(); + } $this->assertEquals(2, $user2->id); } From 4ac0e5a32dfbb7b9dee30988757c4da07131c918 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 03:15:39 +0700 Subject: [PATCH 04/13] use data provider for group by aggregate first --- tests/system/Database/Live/ModelTest.php | 41 ++++++++++++------------ 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index 9f65cfd1e95c..0fccf161778d 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -251,39 +251,40 @@ public function testFirst() //-------------------------------------------------------------------- - public function testFirstWithoutGroupBy() + public function provideGroupBy() { - $model = new UserModel(); - - $user = $model->select('SUM(id) as total') - ->where('id >', 2) - ->first(); - $this->assertEquals(7, $user->total); + return [ + [true], + [false], + ]; } - //-------------------------------------------------------------------- - - public function testFirstWithGroupBy() + /** + * @dataProvider provideGroupBy + */ + public function testFirstAggregate($groupBy) { $model = new UserModel(); - $user = $model->select('SUM(id) as total') + if ($groupBy) + { + $user = $model->select('SUM(id) as total') ->where('id >', 2) ->groupBy('id') ->first(); - $this->assertEquals(3, $user->total); + $this->assertEquals(3, $user->total); + } + else + { + $user = $model->select('SUM(id) as total') + ->where('id >', 2) + ->first(); + $this->assertEquals(7, $user->total); + } } //-------------------------------------------------------------------- - public function provideGroupBy() - { - return [ - [true], - [false], - ]; - } - /** * @dataProvider provideGroupBy */ From 0d272bb6f28a5d6bc70e7a5aa9e62c4348124e52 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 03:38:08 +0700 Subject: [PATCH 05/13] test for aggregate on soft delete --- tests/system/Database/Live/ModelTest.php | 93 +++++++++++++++++------- 1 file changed, 68 insertions(+), 25 deletions(-) diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index 0fccf161778d..32cab9d5021f 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -285,40 +285,72 @@ public function testFirstAggregate($groupBy) //-------------------------------------------------------------------- + public function provideAggregateAndGroupBy() + { + return [ + [ + true, + true, + ], + [ + false, + false, + ], + [ + true, + false, + ], + [ + false, + true, + ], + ]; + } + /** - * @dataProvider provideGroupBy + * @dataProvider provideAggregateAndGroupBy */ - public function testFirstRespectsSoftDeletes($groupBy) + public function testFirstRespectsSoftDeletes($aggregate, $groupBy) { $this->db->table('user') ->where('id', 1) ->update(['deleted_at' => date('Y-m-d H:i:s')]); $model = new UserModel(); - if ($groupBy) + if ($aggregate) { - $user = $model->groupBy('id')->first(); + $model->select('SUM(id) as id'); } - else + + if ($groupBy) { - $user = $model->first(); + $model->groupBy('id'); } - // fix for PHP7.2 - $count = is_array($user) ? count($user) : 1; - $this->assertEquals(1, $count); - $this->assertEquals(2, $user->id); + $user = $model->first(); - $user = $model->withDeleted(); - if ($groupBy) + if (! $aggregate) { - $user = $model->groupBy('id')->first(); + // fix for PHP7.2 + $count = is_array($user) ? count($user) : 1; + $this->assertEquals(1, $count); + $this->assertEquals(2, $user->id); } else { - $user = $model->first(); + if ($groupBy) + { + $this->assertEquals(2, $user->id); + } + else + { + $this->assertEquals(9, $user->id); + } } + $user = $model->withDeleted(); + $user = $model->first(); + $this->assertEquals(1, $user->id); } @@ -1926,30 +1958,41 @@ public function testUndefinedMethodInBuilder() } /** - * @dataProvider provideGroupBy + * @dataProvider provideAggregateAndGroupBy */ - public function testFirstRecoverTempUseSoftDeletes($groupBy) + public function testFirstRecoverTempUseSoftDeletes($aggregate, $groupBy) { $model = new UserModel($this->db); $model->delete(1); - if ($groupBy) + if ($aggregate) { - $user = $model->groupBy('id')->withDeleted()->first(); + $model->select('sum(id) as id'); } - else + + if ($groupBy) { - $user = $model->withDeleted()->first(); + $model->groupBy('id'); } - $this->assertEquals(1, $user->id); - $user2 = $model->first(); - if ($groupBy) + + $user = $model->withDeleted()->first(); + + if (! $aggregate) { - $user2 = $model->groupBy('id')->first(); + $this->assertEquals(1, $user->id); } else { - $user2 = $model->first(); + if ($groupBy) + { + $this->assertEquals(1, $user->id); + } + else + { + $this->assertEquals(10, $user->id); + } } + + $user2 = $model->first(); $this->assertEquals(2, $user2->id); } From 7d515e5a683b3ec9f9cd75e85323aacb588e7e78 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 03:44:50 +0700 Subject: [PATCH 06/13] clean up testFirstAggregate --- tests/system/Database/Live/ModelTest.php | 31 ++++++++++++------------ 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index 32cab9d5021f..e145aee9f7be 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -254,33 +254,34 @@ public function testFirst() public function provideGroupBy() { return [ - [true], - [false], + [ + true, + 3, + ], + [ + false, + 7, + ], ]; } /** * @dataProvider provideGroupBy */ - public function testFirstAggregate($groupBy) + public function testFirstAggregate($groupBy, $total) { $model = new UserModel(); if ($groupBy) { - $user = $model->select('SUM(id) as total') - ->where('id >', 2) - ->groupBy('id') - ->first(); - $this->assertEquals(3, $user->total); - } - else - { - $user = $model->select('SUM(id) as total') - ->where('id >', 2) - ->first(); - $this->assertEquals(7, $user->total); + $model->groupBy('id'); } + + $user = $model->select('SUM(id) as total') + ->where('id >', 2) + ->first(); + + $this->assertEquals($total, $user->total); } //-------------------------------------------------------------------- From d53b351eca2e64590e38aa3a1b4845492832763a Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 04:00:45 +0700 Subject: [PATCH 07/13] clean up test with soft deleted first --- tests/system/Database/Live/ModelTest.php | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index e145aee9f7be..8556d7f42f77 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -330,7 +330,7 @@ public function testFirstRespectsSoftDeletes($aggregate, $groupBy) $user = $model->first(); - if (! $aggregate) + if (! $aggregate || $groupBy) { // fix for PHP7.2 $count = is_array($user) ? count($user) : 1; @@ -339,14 +339,7 @@ public function testFirstRespectsSoftDeletes($aggregate, $groupBy) } else { - if ($groupBy) - { - $this->assertEquals(2, $user->id); - } - else - { - $this->assertEquals(9, $user->id); - } + $this->assertEquals(9, $user->id); } $user = $model->withDeleted(); @@ -1977,20 +1970,13 @@ public function testFirstRecoverTempUseSoftDeletes($aggregate, $groupBy) $user = $model->withDeleted()->first(); - if (! $aggregate) + if (! $aggregate || $groupBy) { $this->assertEquals(1, $user->id); } else { - if ($groupBy) - { - $this->assertEquals(1, $user->id); - } - else - { - $this->assertEquals(10, $user->id); - } + $this->assertEquals(10, $user->id); } $user2 = $model->first(); From 8eadf5d91a5e81401b937efdda59452dfb9c4734 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 04:02:05 +0700 Subject: [PATCH 08/13] clean up test with soft deleted first --- tests/system/Database/Live/ModelTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index 8556d7f42f77..8bf930a52942 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -342,8 +342,8 @@ public function testFirstRespectsSoftDeletes($aggregate, $groupBy) $this->assertEquals(9, $user->id); } - $user = $model->withDeleted(); - $user = $model->first(); + $user = $model->withDeleted() + ->first(); $this->assertEquals(1, $user->id); } From 9c43d761e348c03d62f62992825995ea8a00bdd8 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 04:57:43 +0700 Subject: [PATCH 09/13] soft delete fix first check --- system/Model.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/system/Model.php b/system/Model.php index e02bbe68cd3d..a7f9607e5c89 100644 --- a/system/Model.php +++ b/system/Model.php @@ -473,13 +473,9 @@ public function first() // Some databases, like PostgreSQL, need order // information to consistently return correct results when there is group by - // except when soft delete is enabled - if (empty($builder->QBOrderBy) && ! empty($this->primaryKey)) + if (! empty($this->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) { - if (! empty($this->QBGroupBy) || $this->tempUseSoftDeletes === true) - { - $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); - } + $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); } $row = $builder->limit(1, 0) From e78a89a3599db2d65502213c0629405f35e2a315 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 07:11:22 +0700 Subject: [PATCH 10/13] temporary fix --- system/Database/BaseBuilder.php | 2 +- system/Model.php | 15 +++++++++------ tests/system/Database/Live/ModelTest.php | 4 ++-- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index 9db612a6fd34..33e700458048 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -103,7 +103,7 @@ class BaseBuilder * * @var array */ - protected $QBGroupBy = []; + public $QBGroupBy = []; /** * QB HAVING data diff --git a/system/Model.php b/system/Model.php index a7f9607e5c89..8f3def9782f2 100644 --- a/system/Model.php +++ b/system/Model.php @@ -471,15 +471,18 @@ public function first() $builder->where($this->table . '.' . $this->deletedField, null); } - // Some databases, like PostgreSQL, need order - // information to consistently return correct results when there is group by - if (! empty($this->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) + preg_match('/(MAX\(.+\))|(MIN\(.+\))|(AVG\(.+\))|(SUM\(.+\))|(COUNT\(.+\))/', $builder->getCompiledSelect(false), $matches); + if ($matches) { - $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); + // Some databases, like PostgreSQL, need order + // information to consistently return correct results. + if (! empty($builder->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) + { + $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); + } } - $row = $builder->limit(1, 0) - ->get(); + $row = $builder->limit(1, 0)->get(); $row = $row->getFirstRow($this->tempReturnType); diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index 8bf930a52942..566471bc1577 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -258,10 +258,10 @@ public function provideGroupBy() true, 3, ], - [ + /*[ false, 7, - ], + ],*/ ]; } From 7b2294e60cbadda888dc772760b874e56a5892ab Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 09:06:54 +0700 Subject: [PATCH 11/13] enforce to group by primary key if no group in withDeleted()->first() --- system/Model.php | 20 ++++++++++++-------- tests/system/Database/Live/ModelTest.php | 14 +++----------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/system/Model.php b/system/Model.php index 8f3def9782f2..85492e09286f 100644 --- a/system/Model.php +++ b/system/Model.php @@ -470,19 +470,23 @@ public function first() { $builder->where($this->table . '.' . $this->deletedField, null); } - - preg_match('/(MAX\(.+\))|(MIN\(.+\))|(AVG\(.+\))|(SUM\(.+\))|(COUNT\(.+\))/', $builder->getCompiledSelect(false), $matches); - if ($matches) + else { - // Some databases, like PostgreSQL, need order - // information to consistently return correct results. - if (! empty($builder->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) + if (empty($builder->QBGroupBy) && ! empty($this->primaryKey)) { - $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); + $builder->groupBy($this->table . '.' . $this->primaryKey); } } - $row = $builder->limit(1, 0)->get(); + // Some databases, like PostgreSQL, need order + // information to consistently return correct results + if (! empty($builder->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) + { + $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); + } + + $row = $builder->limit(1, 0) + ->get(); $row = $row->getFirstRow($this->tempReturnType); diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php index 566471bc1577..ba7b6ae6b085 100644 --- a/tests/system/Database/Live/ModelTest.php +++ b/tests/system/Database/Live/ModelTest.php @@ -258,10 +258,10 @@ public function provideGroupBy() true, 3, ], - /*[ + [ false, 7, - ],*/ + ], ]; } @@ -1969,15 +1969,7 @@ public function testFirstRecoverTempUseSoftDeletes($aggregate, $groupBy) } $user = $model->withDeleted()->first(); - - if (! $aggregate || $groupBy) - { - $this->assertEquals(1, $user->id); - } - else - { - $this->assertEquals(10, $user->id); - } + $this->assertEquals(1, $user->id); $user2 = $model->first(); $this->assertEquals(2, $user2->id); From b425afffe51f640fa2d3b9ef370d1479bcec8c35 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 09:12:19 +0700 Subject: [PATCH 12/13] final touch --- system/Model.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Model.php b/system/Model.php index 85492e09286f..c46ad4df29e0 100644 --- a/system/Model.php +++ b/system/Model.php @@ -479,7 +479,7 @@ public function first() } // Some databases, like PostgreSQL, need order - // information to consistently return correct results + // information to consistently return correct results. if (! empty($builder->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey)) { $builder->orderBy($this->table . '.' . $this->primaryKey, 'asc'); From 52b7a088bf367937cd4abe1da85ed8240451b1c6 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Fri, 1 May 2020 09:31:03 +0700 Subject: [PATCH 13/13] add check force group by only when Model::useSoftDeletes is true on Model::first() --- system/Model.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Model.php b/system/Model.php index c46ad4df29e0..ec7365b9c3b0 100644 --- a/system/Model.php +++ b/system/Model.php @@ -472,7 +472,7 @@ public function first() } else { - if (empty($builder->QBGroupBy) && ! empty($this->primaryKey)) + if ($this->useSoftDeletes === true && empty($builder->QBGroupBy) && ! empty($this->primaryKey)) { $builder->groupBy($this->table . '.' . $this->primaryKey); }