Merge pull request #2907 from samsonasik/bug-fix-first-group-by

Fix Model::first() only use orderBy() when group by is not empty
codeigniter4 · May 1, 2020 · 734edb4 · 734edb4
2 parents 08dd1a5 + 52b7a08
commit 734edb4
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 9 deletions.
diff --git 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
@@ -470,16 +470,23 @@ public function first()
 		{
 			$builder->where($this->table . '.' . $this->deletedField, null);
 		}
+		else
+		{
+			if ($this->useSoftDeletes === true && empty($builder->QBGroupBy) && ! empty($this->primaryKey))
+			{
+				$builder->groupBy($this->table . '.' . $this->primaryKey);
+			}
+		}
 
 		// Some databases, like PostgreSQL, need order
 		// information to consistently return correct results.
-		if (empty($builder->QBOrderBy) && ! empty($this->primaryKey))
+		if (! empty($builder->QBGroupBy) && empty($builder->QBOrderBy) && ! empty($this->primaryKey))
 		{
 			$builder->orderBy($this->table . '.' . $this->primaryKey, 'asc');
 		}
 
 		$row = $builder->limit(1, 0)
-				->get();
+					   ->get();
 
 		$row = $row->getFirstRow($this->tempReturnType);
 

diff --git a/tests/system/Database/Live/ModelTest.php b/tests/system/Database/Live/ModelTest.php
@@ -251,20 +251,96 @@ public function testFirst()
 
 	//--------------------------------------------------------------------
 
-	public function testFirstRespectsSoftDeletes()
+	public function provideGroupBy()
+	{
+		return [
+			[
+				true,
+				3,
+			],
+			[
+				false,
+				7,
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider provideGroupBy
+	 */
+	public function testFirstAggregate($groupBy, $total)
+	{
+		$model = new UserModel();
+
+		if ($groupBy)
+		{
+			$model->groupBy('id');
+		}
+
+		$user = $model->select('SUM(id) as total')
+					  ->where('id >', 2)
+					  ->first();
+
+		$this->assertEquals($total, $user->total);
+	}
+
+	//--------------------------------------------------------------------
+
+	public function provideAggregateAndGroupBy()
+	{
+		return [
+			[
+				true,
+				true,
+			],
+			[
+				false,
+				false,
+			],
+			[
+				true,
+				false,
+			],
+			[
+				false,
+				true,
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider provideAggregateAndGroupBy
+	 */
+	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 ($aggregate)
+		{
+			$model->select('SUM(id) as id');
+		}
+
+		if ($groupBy)
+		{
+			$model->groupBy('id');
+		}
 
 		$user = $model->first();
 
-		// fix for PHP7.2
-		$count = is_array($user) ? count($user) : 1;
-		$this->assertEquals(1, $count);
-		$this->assertEquals(2, $user->id);
+		if (! $aggregate || $groupBy)
+		{
+			// fix for PHP7.2
+			$count = is_array($user) ? count($user) : 1;
+			$this->assertEquals(1, $count);
+			$this->assertEquals(2, $user->id);
+		}
+		else
+		{
+			$this->assertEquals(9, $user->id);
+		}
 
 		$user = $model->withDeleted()
 					  ->first();
@@ -1875,12 +1951,26 @@ public function testUndefinedMethodInBuilder()
 			->getBindings();
 	}
 
-	public function testFirstRecoverTempUseSoftDeletes()
+	/**
+	 * @dataProvider provideAggregateAndGroupBy
+	 */
+	public function testFirstRecoverTempUseSoftDeletes($aggregate, $groupBy)
 	{
 		$model = new UserModel($this->db);
 		$model->delete(1);
+		if ($aggregate)
+		{
+			$model->select('sum(id) as id');
+		}
+
+		if ($groupBy)
+		{
+			$model->groupBy('id');
+		}
+
 		$user = $model->withDeleted()->first();
 		$this->assertEquals(1, $user->id);
+
 		$user2 = $model->first();
 		$this->assertEquals(2, $user2->id);
 	}