Skip to content

[10.x] Fix CanBeOneOfMany giving erroneous results #47427

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

Merged
merged 15 commits into from
Sep 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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})";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why min here? Can you explain the entire logic behind this PR and how it fixes the issue?

Copy link
Contributor Author

@Guilhem-DELAITRE Guilhem-DELAITRE Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The min purpose is to use an aggregate to select any value on a column which we already know all value are the same because it's the sub-level aggregate. We still need an aggregate because there's no group by clause on that column. The most correct solution would probably be to add a group by on that column, but I didn't manage to do it in the time I had.

The more global purpose of the PR is fully stated and detailed (and is demonstrated by the modification of the generated query) in the original PR : #46309

The last two commits purpose is to fix the problem mentionned in
#46394 which was due to my modification not working with DBRMS which are strict with aggregates / group by like PostgreSQL.

}

$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 = [];
}