FIX on CanBeOneOfMany trait giving erroneous results (#46309)

* Add test to pinpoint the dysfunction * FIX CanBeOneOfMany.php * FIX test on raw sql
laravel · Mar 6, 2023 · 494a417 · 494a417
1 parent 66a619d
commit 494a417
Show file tree

Hide file tree

Showing 2 changed files with 60 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,19 @@ 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.')';
+                }
+
+                $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 +222,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 +231,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", "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