From 05c7572779746eb30229c52a329b9a079e54c9b1 Mon Sep 17 00:00:00 2001 From: Guilhem-DELAITRE <89917125+Guilhem-DELAITRE@users.noreply.github.com> Date: Thu, 21 Sep 2023 23:06:49 +0200 Subject: [PATCH] [10.x] Fix `CanBeOneOfMany` giving erroneous results (#47427) * 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 --- .../Relations/Concerns/CanBeOneOfMany.php | 43 ++++++++--- .../DatabaseEloquentHasOneOfManyTest.php | 31 +++++++- .../Database/EloquentHasOneOfManyTest.php | 71 +++++++++++++++++++ 3 files changed, 133 insertions(+), 12 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php b/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php index 25b9a5834bad..7e3babe3fc52 100644 --- a/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php +++ b/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php @@ -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)) { @@ -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'] ?? []), ]; } @@ -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|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() @@ -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; } @@ -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 $on * @return void */ protected function addOneOfManyJoinSubQuery(Builder $parent, Builder $subQuery, $on) @@ -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); }); diff --git a/tests/Database/DatabaseEloquentHasOneOfManyTest.php b/tests/Database/DatabaseEloquentHasOneOfManyTest.php index af5e620f5712..2f564da2172b 100755 --- a/tests/Database/DatabaseEloquentHasOneOfManyTest.php +++ b/tests/Database/DatabaseEloquentHasOneOfManyTest.php @@ -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) { }); @@ -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. * @@ -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 diff --git a/tests/Integration/Database/EloquentHasOneOfManyTest.php b/tests/Integration/Database/EloquentHasOneOfManyTest.php index 2f0b774ce401..4be71d3e5da7 100644 --- a/tests/Integration/Database/EloquentHasOneOfManyTest.php +++ b/tests/Integration/Database/EloquentHasOneOfManyTest.php @@ -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() @@ -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 @@ -55,6 +90,37 @@ 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 @@ -62,3 +128,8 @@ class Login extends Model protected $guarded = []; public $timestamps = false; } + +class State extends Model +{ + protected $guarded = []; +}