Skip to content

Commit

Permalink
Revert "FIX on CanBeOneOfMany trait giving erroneous results (#46309)" (
Browse files Browse the repository at this point in the history 
#46402)

This reverts commit 494a417.
  • Loading branch information
@strotgen
strotgen authored Mar 8, 2023
1 parent 4964a1a commit 496d12d
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 60 deletions.
41 changes: 11 additions & 30 deletions src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,11 @@ public function ofMany($column = 'id', $aggregate = 'MAX', $relation = null)

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

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

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

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

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

Expand Down Expand Up @@ -186,11 +177,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 array<string>|null $columns
* @param string|null $column
* @param string|null $aggregate
* @return \Illuminate\Database\Eloquent\Builder
*/
protected function newOneOfManySubQuery($groupBy, $columns = null, $aggregate = null)
protected function newOneOfManySubQuery($groupBy, $column = null, $aggregate = null)
{
$subQuery = $this->query->getModel()
->newQuery()
Expand All @@ -200,19 +191,11 @@ protected function newOneOfManySubQuery($groupBy, $columns = null, $aggregate =
$subQuery->groupBy($this->qualifyRelatedColumn($group));
}

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'));
}
if (! is_null($column)) {
$subQuery->selectRaw($aggregate.'('.$subQuery->getQuery()->grammar->wrap($subQuery->qualifyColumn($column)).') as '.$subQuery->getQuery()->grammar->wrap($column.'_aggregate'));
}

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

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

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

$this->addOneOfManyJoinSubQueryConstraints($join, $on);
});
Expand Down
31 changes: 1 addition & 30 deletions tests/Database/DatabaseEloquentHasOneOfManyTest.php
View file
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"."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());
$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());

HasOneOfManyTestPrice::addGlobalScope('test', function ($query) {
});
Expand Down Expand Up @@ -486,27 +486,6 @@ 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 @@ -642,14 +621,6 @@ 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

1 comment on commit 496d12d

@smecnarowski
Copy link

Choose a reason for hiding this comment

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

framework/src/Illuminate/Database/Eloquent/Relations/Concerns/CanBeOneOfMany.php

Line 77 in c3e679c

$keyName = $this->query->getModel()->getKeyName();
what about if in model $primaryKey is null ? $keyName = $this->query->getModel()->getKeyName(); will became an empty array key in $columns array. Finally it will generate SQL with bad syntax where empty column name will be in query.
Mentioned here #46463 - ignored because of unsupported version but I mentioned that i still exists in a code 10.x

Please sign in to comment.