Skip to content

Commit

Permalink
[10.x] Fix CanBeOneOfMany giving erroneous results (#47427)
Browse files Browse the repository at this point in the history
* Add test to pinpoint the dysfunction

* FIX CanBeOneOfMany.php

* FIX test on raw sql

* FIX missing aggregate for strict RDBMS (like postgresql)

* FIX

* Add integration test to ensure multi-databases coverage

* Update EloquentHasOneOfManyTest.php

* Update EloquentHasOneOfManyTest.php

* wip

* wip

* wip

* wip

* wip

---------

Co-authored-by: Mior Muhammad Zaki <[email protected]>
  • Loading branch information
Guilhem-DELAITRE and crynobone authored Sep 21, 2023
1 parent 19357ef commit 05c7572
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,16 @@ public function ofMany($column = 'id', $aggregate = 'MAX', $relation = null)

$subQuery = $this->newOneOfManySubQuery(
$this->getOneOfManySubQuerySelectColumns(),
$column, $aggregate
array_merge([$column], $previous['columns'] ?? []),
$aggregate,
);

if (isset($previous)) {
$this->addOneOfManyJoinSubQuery($subQuery, $previous['subQuery'], $previous['column']);
$this->addOneOfManyJoinSubQuery(
$subQuery,
$previous['subQuery'],
$previous['columns'],
);
}

if (isset($closure)) {
Expand All @@ -112,12 +117,16 @@ public function ofMany($column = 'id', $aggregate = 'MAX', $relation = null)
}

if (array_key_last($columns) == $column) {
$this->addOneOfManyJoinSubQuery($this->query, $subQuery, $column);
$this->addOneOfManyJoinSubQuery(
$this->query,
$subQuery,
array_merge([$column], $previous['columns'] ?? []),
);
}

$previous = [
'subQuery' => $subQuery,
'column' => $column,
'columns' => array_merge([$column], $previous['columns'] ?? []),
];
}

Expand Down Expand Up @@ -177,11 +186,11 @@ protected function getDefaultOneOfManyJoinAlias($relation)
* Get a new query for the related model, grouping the query by the given column, often the foreign key of the relationship.
*
* @param string|array $groupBy
* @param string|null $column
* @param array<string>|null $columns
* @param string|null $aggregate
* @return \Illuminate\Database\Eloquent\Builder
*/
protected function newOneOfManySubQuery($groupBy, $column = null, $aggregate = null)
protected function newOneOfManySubQuery($groupBy, $columns = null, $aggregate = null)
{
$subQuery = $this->query->getModel()
->newQuery()
Expand All @@ -191,11 +200,21 @@ protected function newOneOfManySubQuery($groupBy, $column = null, $aggregate = n
$subQuery->groupBy($this->qualifyRelatedColumn($group));
}

if (! is_null($column)) {
$subQuery->selectRaw($aggregate.'('.$subQuery->getQuery()->grammar->wrap($subQuery->qualifyColumn($column)).') as '.$subQuery->getQuery()->grammar->wrap($column.'_aggregate'));
if (! is_null($columns)) {
foreach ($columns as $key => $column) {
$aggregatedColumn = $subQuery->getQuery()->grammar->wrap($subQuery->qualifyColumn($column));

if ($key === 0) {
$aggregatedColumn = "{$aggregate}({$aggregatedColumn})";
} else {
$aggregatedColumn = "min({$aggregatedColumn})";
}

$subQuery->selectRaw($aggregatedColumn.' as '.$subQuery->getQuery()->grammar->wrap($column.'_aggregate'));
}
}

$this->addOneOfManySubQueryConstraints($subQuery, $groupBy, $column, $aggregate);
$this->addOneOfManySubQueryConstraints($subQuery, $groupBy, $columns, $aggregate);

return $subQuery;
}
Expand All @@ -205,7 +224,7 @@ protected function newOneOfManySubQuery($groupBy, $column = null, $aggregate = n
*
* @param \Illuminate\Database\Eloquent\Builder $parent
* @param \Illuminate\Database\Eloquent\Builder $subQuery
* @param string $on
* @param array<string> $on
* @return void
*/
protected function addOneOfManyJoinSubQuery(Builder $parent, Builder $subQuery, $on)
Expand All @@ -214,7 +233,9 @@ protected function addOneOfManyJoinSubQuery(Builder $parent, Builder $subQuery,
$subQuery->applyBeforeQueryCallbacks();

$parent->joinSub($subQuery, $this->relationName, function ($join) use ($on) {
$join->on($this->qualifySubSelectColumn($on.'_aggregate'), '=', $this->qualifyRelatedColumn($on));
foreach ($on as $onColumn) {
$join->on($this->qualifySubSelectColumn($onColumn.'_aggregate'), '=', $this->qualifyRelatedColumn($onColumn));
}

$this->addOneOfManyJoinSubQueryConstraints($join, $on);
});
Expand Down
31 changes: 30 additions & 1 deletion tests/Database/DatabaseEloquentHasOneOfManyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public function testGlobalScopeIsNotAppliedWhenRelationIsDefinedWithoutGlobalSco

$user = HasOneOfManyTestUser::create();
$relation = $user->price_without_global_scope();
$this->assertSame('select "prices".* from "prices" inner join (select max("prices"."id") as "id_aggregate", "prices"."user_id" from "prices" inner join (select max("prices"."published_at") as "published_at_aggregate", "prices"."user_id" from "prices" where "published_at" < ? and "prices"."user_id" = ? and "prices"."user_id" is not null group by "prices"."user_id") as "price_without_global_scope" on "price_without_global_scope"."published_at_aggregate" = "prices"."published_at" and "price_without_global_scope"."user_id" = "prices"."user_id" where "published_at" < ? group by "prices"."user_id") as "price_without_global_scope" on "price_without_global_scope"."id_aggregate" = "prices"."id" and "price_without_global_scope"."user_id" = "prices"."user_id" where "prices"."user_id" = ? and "prices"."user_id" is not null', $relation->getQuery()->toSql());
$this->assertSame('select "prices".* from "prices" inner join (select max("prices"."id") as "id_aggregate", min("prices"."published_at") as "published_at_aggregate", "prices"."user_id" from "prices" inner join (select max("prices"."published_at") as "published_at_aggregate", "prices"."user_id" from "prices" where "published_at" < ? and "prices"."user_id" = ? and "prices"."user_id" is not null group by "prices"."user_id") as "price_without_global_scope" on "price_without_global_scope"."published_at_aggregate" = "prices"."published_at" and "price_without_global_scope"."user_id" = "prices"."user_id" where "published_at" < ? group by "prices"."user_id") as "price_without_global_scope" on "price_without_global_scope"."id_aggregate" = "prices"."id" and "price_without_global_scope"."published_at_aggregate" = "prices"."published_at" and "price_without_global_scope"."user_id" = "prices"."user_id" where "prices"."user_id" = ? and "prices"."user_id" is not null', $relation->getQuery()->toSql());

HasOneOfManyTestPrice::addGlobalScope('test', function ($query) {
});
Expand Down Expand Up @@ -486,6 +486,27 @@ public function testWithContraintNotInAggregate()
$this->assertSame($newFoo->id, $user->last_updated_foo_state->id);
}

public function testItGetsCorrectResultUsingAtLeastTwoAggregatesDistinctFromId()
{
$user = HasOneOfManyTestUser::create();

$expectedState = $user->states()->create([
'state' => 'state',
'type' => 'type',
'created_at' => '2023-01-01',
'updated_at' => '2023-01-03',
]);

$user->states()->create([
'state' => 'state',
'type' => 'type',
'created_at' => '2023-01-01',
'updated_at' => '2023-01-02',
]);

$this->assertSame($user->latest_updated_latest_created_state->id, $expectedState->id);
}

/**
* Get a database connection instance.
*
Expand Down Expand Up @@ -621,6 +642,14 @@ public function price_without_global_scope()
$q->where('published_at', '<', now());
});
}

public function latest_updated_latest_created_state()
{
return $this->hasOne(HasOneOfManyTestState::class, 'user_id')->ofMany([
'updated_at' => 'max',
'created_at' => 'max',
]);
}
}

class HasOneOfManyTestModel extends Eloquent
Expand Down
71 changes: 71 additions & 0 deletions tests/Integration/Database/EloquentHasOneOfManyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ protected function defineDatabaseMigrationsAfterDatabaseRefreshed()
$table->id();
$table->foreignId('user_id');
});

Schema::create('states', function ($table) {
$table->increments('id');
$table->string('state');
$table->string('type');
$table->foreignId('user_id');
$table->timestamps();
});
}

public function testItOnlyEagerLoadsRequiredModels()
Expand All @@ -44,6 +52,33 @@ public function testItOnlyEagerLoadsRequiredModels()

$this->assertSame(2, $this->retrievedLogins);
}

public function testItGetsCorrectResultUsingAtLeastTwoAggregatesDistinctFromId()
{
$user = User::create();

$latestState = $user->states()->create([
'state' => 'state',
'type' => 'type',
'created_at' => '2023-01-01',
'updated_at' => '2023-01-03',
]);

$oldestState = $user->states()->create([
'state' => 'state',
'type' => 'type',
'created_at' => '2023-01-01',
'updated_at' => '2023-01-02',
]);

$this->assertSame($user->oldest_updated_state->id, $oldestState->id);
$this->assertSame($user->oldest_updated_oldest_created_state->id, $oldestState->id);

$users = User::with('latest_updated_state', 'latest_updated_latest_created_state')->get();

$this->assertSame($users[0]->latest_updated_state->id, $latestState->id);
$this->assertSame($users[0]->latest_updated_latest_created_state->id, $latestState->id);
}
}

class User extends Model
Expand All @@ -55,10 +90,46 @@ public function latest_login()
{
return $this->hasOne(Login::class)->ofMany();
}

public function states()
{
return $this->hasMany(State::class);
}

public function latest_updated_state()
{
return $this->hasOne(State::class, 'user_id')->ofMany('updated_at', 'max');
}

public function oldest_updated_state()
{
return $this->hasOne(State::class, 'user_id')->ofMany('updated_at', 'min');
}

public function latest_updated_latest_created_state()
{
return $this->hasOne(State::class, 'user_id')->ofMany([
'updated_at' => 'max',
'created_at' => 'max',
]);
}

public function oldest_updated_oldest_created_state()
{
return $this->hasOne(State::class, 'user_id')->ofMany([
'updated_at' => 'min',
'created_at' => 'min',
]);
}
}

class Login extends Model
{
protected $guarded = [];
public $timestamps = false;
}

class State extends Model
{
protected $guarded = [];
}

0 comments on commit 05c7572

Please sign in to comment.