[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 <[email protected]>
laravel · Sep 22, 2023 · 86b8928 · 86b8928
1 parent f19c46f
commit 86b8928
Show file tree

Hide file tree

Showing 3 changed files with 133 additions and 12 deletions.
diff --git 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<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()
@@ -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<string>  $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
@@ -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
@@ -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,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 = [];
+}