Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[10.x] Fix expressions in with-functions doing aggregates #49912

Merged
merged 3 commits into from
Jan 30, 2024
Merged

[10.x] Fix expressions in with-functions doing aggregates #49912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eb1099
support expression in with-functions doing aggregates
tpetry Jan 30, 2024
60254a0
apply Laravel docblock rules
tpetry Jan 30, 2024
92e40f0
typo
tpetry Jan 30, 2024
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
28 changes: 16 additions & 12 deletions src/Illuminate/Database/Eloquent/Concerns/QueriesRelationships.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,7 @@ public function orWhereBelongsTo($related, $relationshipName = null)
* Add subselect queries to include an aggregate value for a relationship.
*
* @param mixed $relations
* @param string $column
* @param \Illuminate\Contracts\Database\Query\Expression|string $column
* @param string $function
* @return $this
*/
Expand Down Expand Up @@ -630,15 +630,19 @@ public function withAggregate($relations, $column, $function = null)
$relation = $this->getRelationWithoutConstraints($name);

if ($function) {
$hashedColumn = $this->getRelationHashedColumn($column, $relation);
if ($this->getGrammar()->isExpression($column)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @tpetry please correct me if I'm wrong, but I believe this should be $this->getQuery()->getGrammar(), instead of $this->getGrammar() directly, right?

$aggregateColumn = $this->getGrammar()->getValue($column);
} else {
$hashedColumn = $this->getRelationHashedColumn($column, $relation);

$wrappedColumn = $this->getQuery()->getGrammar()->wrap(
$column === '*' ? $column : $relation->getRelated()->qualifyColumn($hashedColumn)
);
$aggregateColumn = $this->getQuery()->getGrammar()->wrap(
$column === '*' ? $column : $relation->getRelated()->qualifyColumn($hashedColumn)
);
}

$expression = $function === 'exists' ? $wrappedColumn : sprintf('%s(%s)', $function, $wrappedColumn);
$expression = $function === 'exists' ? $aggregateColumn : sprintf('%s(%s)', $function, $aggregateColumn);
} else {
$expression = $column;
$expression = $this->getGrammar()->getValue($column);
}

// Here, we will grab the relationship sub-query and prepare to add it to the main query
Expand Down Expand Up @@ -667,7 +671,7 @@ public function withAggregate($relations, $column, $function = null)
// the query builder. Then, we will return the builder instance back to the developer
// for further constraint chaining that needs to take place on the query as needed.
$alias ??= Str::snake(
preg_replace('/[^[:alnum:][:space:]_]/u', '', "$name $function $column")
preg_replace('/[^[:alnum:][:space:]_]/u', '', "$name $function {$this->getGrammar()->getValue($column)}")
);

if ($function === 'exists') {
Expand Down Expand Up @@ -719,7 +723,7 @@ public function withCount($relations)
* Add subselect queries to include the max of the relation's column.
*
* @param string|array $relation
* @param string $column
* @param \Illuminate\Contracts\Database\Query\Expression|string $column
* @return $this
*/
public function withMax($relation, $column)
Expand All @@ -731,7 +735,7 @@ public function withMax($relation, $column)
* Add subselect queries to include the min of the relation's column.
*
* @param string|array $relation
* @param string $column
* @param \Illuminate\Contracts\Database\Query\Expression|string $column
* @return $this
*/
public function withMin($relation, $column)
Expand All @@ -743,7 +747,7 @@ public function withMin($relation, $column)
* Add subselect queries to include the sum of the relation's column.
*
* @param string|array $relation
* @param string $column
* @param \Illuminate\Contracts\Database\Query\Expression|string $column
* @return $this
*/
public function withSum($relation, $column)
Expand All @@ -755,7 +759,7 @@ public function withSum($relation, $column)
* Add subselect queries to include the average of the relation's column.
*
* @param string|array $relation
* @param string $column
* @param \Illuminate\Contracts\Database\Query\Expression|string $column
* @return $this
*/
public function withAvg($relation, $column)
Expand Down
12 changes: 12 additions & 0 deletions tests/Database/DatabaseEloquentBelongsToManyAggregateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Database\Capsule\Manager as DB;
use Illuminate\Database\Eloquent\Model as Eloquent;
use Illuminate\Database\Query\Expression;
use PHPUnit\Framework\TestCase;

class DatabaseEloquentBelongsToManyAggregateTest extends TestCase
Expand Down Expand Up @@ -45,6 +46,17 @@ public function testWithSumSameTable()
$this->assertEquals(1200, $order->total_allocated);
}

public function testWithSumExpression()
{
$this->seedData();

$order = BelongsToManyAggregateTestTestTransaction::query()
->withSum('allocatedTo as total_allocated', new Expression('allocations.amount * 2'))
->first();

$this->assertEquals(2400, $order->total_allocated);
}

/**
* Setup the database schema.
*
Expand Down
46 changes: 46 additions & 0 deletions tests/Database/DatabaseEloquentBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Database\Eloquent\SoftDeletes;
use Illuminate\Database\Query\Builder as BaseBuilder;
use Illuminate\Database\Query\Expression;
use Illuminate\Database\Query\Grammars\Grammar;
use Illuminate\Database\Query\Processors\Processor;
use Illuminate\Support\Carbon;
Expand Down Expand Up @@ -1278,6 +1279,15 @@ public function testWithMin()
$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select min("eloquent_builder_test_model_close_related_stubs"."price") from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id") as "foo_min_price" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

public function testWithMinExpression()
{
$model = new EloquentBuilderTestModelParentStub;

$builder = $model->withMin('foo', new Expression('price - discount'));

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select min(price - discount) from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id") as "foo_min_price_discount" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

public function testWithMinOnBelongsToMany()
{
$model = new EloquentBuilderTestModelParentStub;
Expand All @@ -1302,6 +1312,42 @@ public function testWithMinOnSelfRelated()
$this->assertSame('select "self_related_stubs".*, (select min("self_alias_hash"."created_at") from "self_related_stubs" as "self_alias_hash" where "self_related_stubs"."id" = "self_alias_hash"."parent_id") as "child_foos_min_created_at" from "self_related_stubs"', $sql);
}

public function testWithMax()
{
$model = new EloquentBuilderTestModelParentStub;

$builder = $model->withMax('foo', 'price');

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select max("eloquent_builder_test_model_close_related_stubs"."price") from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id") as "foo_max_price" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

public function testWithMaxExpression()
{
$model = new EloquentBuilderTestModelParentStub;

$builder = $model->withMax('foo', new Expression('price - discount'));

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select max(price - discount) from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id") as "foo_max_price_discount" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

public function testWithAvg()
{
$model = new EloquentBuilderTestModelParentStub;

$builder = $model->withAvg('foo', 'price');

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select avg("eloquent_builder_test_model_close_related_stubs"."price") from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id") as "foo_avg_price" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

public function testWitAvgExpression()
{
$model = new EloquentBuilderTestModelParentStub;

$builder = $model->withAvg('foo', new Expression('price - discount'));

$this->assertSame('select "eloquent_builder_test_model_parent_stubs".*, (select avg(price - discount) from "eloquent_builder_test_model_close_related_stubs" where "eloquent_builder_test_model_parent_stubs"."foo_id" = "eloquent_builder_test_model_close_related_stubs"."id") as "foo_avg_price_discount" from "eloquent_builder_test_model_parent_stubs"', $builder->toSql());
}

public function testWithCountAndConstraintsAndHaving()
{
$model = new EloquentBuilderTestModelParentStub;
Expand Down
Loading