Skip to content
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.

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

Fix Model::first() only use orderBy() when group by is not empty #2907

Merged
merged 13 commits into from
May 1, 2020
Merged
2 changes: 1 addition & 1 deletion system/Database/BaseBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class BaseBuilder
*
* @var array
*/
protected $QBGroupBy = [];
public $QBGroupBy = [];

/**
* QB HAVING data
Expand Down
11 changes: 9 additions & 2 deletions system/Model.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
102 changes: 96 additions & 6 deletions tests/system/Database/Live/ModelTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
Expand Down