Skip to content

[5.8] Add aggregates eager loading support #26481

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.

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions src/Illuminate/Database/Eloquent/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -1107,9 +1107,10 @@ public function newModelInstance($attributes = [])
* Parse a list of relations into individuals.
*
* @param array $relations
* @param bool $selectConstraints
* @return array
*/
protected function parseWithRelations(array $relations)
protected function parseWithRelations(array $relations, $selectConstraints = true)
{
$results = [];

Expand All @@ -1120,7 +1121,7 @@ protected function parseWithRelations(array $relations)
if (is_numeric($name)) {
$name = $constraints;

[$name, $constraints] = Str::contains($name, ':')
[$name, $constraints] = $selectConstraints && Str::contains($name, ':')
? $this->createSelectWithConstraint($name)
: [$name, function () {
//
Expand Down
114 changes: 104 additions & 10 deletions src/Illuminate/Database/Eloquent/Concerns/QueriesRelationships.php
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,74 @@ public function orWhereDoesntHave($relation, Closure $callback = null)
* @param mixed $relations
* @return $this
*/
public function withCount($relations)
public function withCount(...$relations)
{
return $this->withAggregate('count', ...$relations);
}

/**
* Add subselect queries to retrieve the minimum value of a column on the relations.
*
* @param mixed $relations
* @return $this
*/
public function withMin(...$relations)
{
return $this->withAggregate('min', ...$relations);
}

/**
* Add subselect queries to retrieve the maximum value of a column on the relations.
*
* @param mixed $relations
* @return $this
*/
public function withMax(...$relations)
{
return $this->withAggregate('max', ...$relations);
}

/**
* Add subselect queries to retrieve the sum the value of a column on the relations.
*
* @param mixed $relations
* @return $this
*/
public function withSum(...$relations)
{
return $this->withAggregate('sum', ...$relations);
}

/**
* Add subselect queries to retrieve the average the value of a column on the relations.
*
* @param mixed $relations
* @return $this
*/
public function withAvg(...$relations)
{
return $this->withAggregate('avg', ...$relations);
}

/**
* Alias for the "withAvg" method.
*
* @param mixed $relations
* @return $this
*/
public function withAverage(...$relations)
{
return $this->withAvg(...$relations);
}

/**
* Add subselect queries to retrieve the aggregate function of a column on the relations.
*
* @param string $function
* @param mixed $relations
* @return $this
*/
public function withAggregate($function, $relations)
{
if (empty($relations)) {
return $this;
Expand All @@ -198,12 +265,12 @@ public function withCount($relations)
$this->query->select([$this->query->from.'.*']);
}

$relations = is_array($relations) ? $relations : func_get_args();
$relations = is_array($relations) ? $relations : array_slice(func_get_args(), 1);

foreach ($this->parseWithRelations($relations) as $name => $constraints) {
foreach ($this->parseWithRelations($relations, false) as $name => $constraints) {
// First we will determine if the name has been aliased using an "as" clause on the name
// and if it has we will extract the actual relationship name and the desired name of
// the resulting column. This allows multiple counts on the same relationship name.
// the resulting column. This allows multiple aggregates for the same relationship.
$segments = explode(' ', $name);

unset($alias);
Expand All @@ -212,13 +279,22 @@ public function withCount($relations)
[$name, $alias] = [$segments[0], $segments[2]];
}

// Determine column to do the aggregate function
$segments = explode(':', $name);
Copy link
Member

Choose a reason for hiding this comment

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

Need explanation for these 4 lines or so... no idea what the purpose is and comment is not clear at all.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I thought the code was self-explanatory. The next lines give us an idea of what they do:

if (count($segments) === 2) {
    [$name, $aggregateColumn] = $segments;
}

The purpose is to extract the relation name and the column name. For example, calling User::withSum('posts:earnings') would mean that we want to retrieve users with a property called post_earnings_sum (that's the default name, it can be changed with AS something) whose value is the earnings column sum of the posts relation


unset($aggregateColumn);

if (count($segments) === 2) {
[$name, $aggregateColumn] = $segments;
}

$relation = $this->getRelationWithoutConstraints($name);

// Here we will get the relationship count query and prepare to add it to the main query
// as a sub-select. First, we'll get the "has" query and use that to get the relation
// count query. We will normalize the relation name then append _count as the name.
$query = $relation->getRelationExistenceCountQuery(
$relation->getRelated()->newQuery(), $this
// Here we will get the relationship aggregate query and prepare to add it to the main query
// as a sub-select. First we'll get the "has" query and use that to retrieve the relation
// aggregate query. We normalize the relation name, and append _$column_$aggregateName.
$query = $relation->getRelationExistenceAggregateQuery(
$relation->getRelated()->newQuery(), $this, $function, $aggregateColumn ?? '*'
);

$query->callScope($constraints);
Expand All @@ -232,14 +308,32 @@ public function withCount($relations)
// Finally we will add the proper result column alias to the query and run the subselect
// statement against the query builder. Then we will return the builder instance back
// to the developer for further constraint chaining that needs to take place on it.
$column = $alias ?? Str::snake($name.'_count');
$suffix = isset($aggregateColumn) ? '_'.$aggregateColumn : null;
$column = $alias ?? Str::snake($name.$suffix.'_'.$function);

$this->selectSub($query, $column);
}

return $this;
}

/**
* Apply a set of aggregate functions of related model.
*
* @param array $aggregates
* @return $this
*/
public function withAggregates($aggregates)
{
foreach ($aggregates as $function => $columns) {
foreach ((array) $columns as $column) {
$this->withAggregate($function, $column);
}
}

return $this;
}

/**
* Add the "has" condition where clause to the query.
*
Expand Down
10 changes: 9 additions & 1 deletion src/Illuminate/Database/Eloquent/Model.php
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,13 @@ abstract class Model implements ArrayAccess, Arrayable, Jsonable, JsonSerializab
*/
protected $withCount = [];

/**
* The relationship aggregates that should be eager loaded on every query.
*
* @var array
*/
protected $withAggregates = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Another option would be to remove withCount – it's a bigger breaking change, but I think might make more sense, so we don't leave two ways of doing the same thing – feels like we run the risk of more edge cases leaving it this way

Copy link
Contributor

@deleugpn deleugpn Dec 4, 2018

Choose a reason for hiding this comment

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

I would be against removing withCount without proper deprecation to give people at least 1 release cycle to work it out slowly. It's a relevant change that could impact packages, tutorials, documentations, a lot of content overall, not just people's application.
Getting aggregates in is something that I've want for a long time now and there's been several attempts already. I'd say let's focus on getting it to work and once it's in and stable we can consider removing withCount separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a deprecation notice, that seems like the only sensible way forward.


/**
* The number of models to return for pagination.
*
Expand Down Expand Up @@ -987,7 +994,8 @@ public function newQueryWithoutScopes()
{
return $this->newModelQuery()
->with($this->with)
->withCount($this->withCount);
->withCount($this->withCount)
->withAggregates($this->withAggregates);
}

/**
Expand Down
18 changes: 17 additions & 1 deletion src/Illuminate/Database/Eloquent/Relations/Relation.php
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,24 @@ public function rawUpdate(array $attributes = [])
*/
public function getRelationExistenceCountQuery(Builder $query, Builder $parentQuery)
{
return $this->getRelationExistenceAggregateQuery($query, $parentQuery, 'count', '*');
}

/**
* Add the constraints for a relationship aggregate query.
*
* @param \Illuminate\Database\Eloquent\Builder $query
* @param \Illuminate\Database\Eloquent\Builder $parentQuery
* @param string $function
* @param string $column
* @return \Illuminate\Database\Eloquent\Builder
*/
public function getRelationExistenceAggregateQuery(Builder $query, Builder $parentQuery, $function, $column)
{
$wrappedColumn = $query->getQuery()->getGrammar()->wrap($column);

return $this->getRelationExistenceQuery(
$query, $parentQuery, new Expression('count(*)')
$query, $parentQuery, new Expression($function.'('.$wrappedColumn.')')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there's any value in keeping a list of valid functions? So that we can tell the user that aggregate is not supported, otherwise I worry users might end up with domain code that works with one database engine and not another (which can lead to obscure bugs). It would be better that users be very explicit when they choose to use an aggregation that their particular database supports.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd guess the database error message would be clear enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, my concern is database compatibility – if you have to switch from one database to another for some reason, and your new database doesn't support the same aggregation (or worse, for some reason has a slightly different syntax), you'd be in a world of pain. I'm not sure this is anything that might happen, but I did want to bring it up just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Last year I was part of switching a mid-sized Laravel project from MySQL to Postgres.

I don't remember any Eloquent-related code paths had to be touched. Only MySQL specific SQL statements (💥 ).

To sum it up: this kind of thinking is very much appreciated!

Copy link
Author

Choose a reason for hiding this comment

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

I agree with @deleugpn. What's the benefit of keeping a list of valid functions? In other words, how it would improve the user experience?

Having a list of valid functions or not, the error only would be triggered when the query is executed

Copy link
Contributor

Choose a reason for hiding this comment

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

@storrella if developers have a discrepancy between their production and dev environment, where they might run MySQL 8 locally and MySQL 5.6 in prod, they could run into issues. Those issues would be discovered at time of dev if you had a whitelist. Docker helps with this, but I don't think it's necessarily fair to assume a perfect dev setup either. Just feels like one of those things that's likely to go wrong when we let users input literally anything.

Copy link
Contributor

@deleugpn deleugpn Dec 5, 2018

Choose a reason for hiding this comment

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

Wouldn't we end up whitelisting both the MySQL 8 function as well as the MySQL 5.6 and be back to square 1? I guess like @storrella I'm having difficulties understanding the benefit of this as well since it's on Eloquent side. This isn't on a driver-specific Grammar class, so there's no point in whitelisting anything because you would end up whitelisting every function of every supported driver. Whatever error you get here you would get with your database as well. The difference is we would need to whitelist functions and map to a specific error message. Instead, let's not whitelist anything and let the database explain what went wrong.

)->setBindings([], 'select');
}

Expand Down
8 changes: 7 additions & 1 deletion tests/Database/DatabaseEloquentHasOneTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Query\Expression;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Query\Grammars\Grammar;
use Illuminate\Database\Eloquent\Relations\HasOne;
use Illuminate\Database\Query\Builder as BaseBuilder;

Expand Down Expand Up @@ -206,9 +207,14 @@ public function testRelationCountQueryCanBeBuilt()
$parentQuery = m::mock(BaseBuilder::class);
$parentQuery->from = 'two';

$builder->shouldReceive('getQuery')->once()->andReturn($baseQuery);
$grammar = m::mock(Grammar::class);

$builder->shouldReceive('getQuery')->twice()->andReturn($baseQuery);
$builder->shouldReceive('getQuery')->once()->andReturn($parentQuery);

$grammar->shouldReceive('wrap')->once()->with('*')->andReturn('*');
$baseQuery->shouldReceive('getGrammar')->once()->andReturn($grammar);

$builder->shouldReceive('select')->once()->with(m::type(Expression::class))->andReturnSelf();
$relation->getParent()->shouldReceive('qualifyColumn')->andReturn('table.id');
$builder->shouldReceive('whereColumn')->once()->with('table.id', '=', 'table.foreign_key')->andReturn($baseQuery);
Expand Down
148 changes: 148 additions & 0 deletions tests/Integration/Database/EloquentWithAggregateTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
<?php

namespace Illuminate\Tests\Integration\Database\EloquentWithAggregateTest;

use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Tests\Integration\Database\DatabaseTestCase;

/**
* @group integration
*/
class EloquentWithAggregateTest extends DatabaseTestCase
{
public function setUp()
{
parent::setUp();

Schema::create('one', function ($table) {
$table->increments('id');
});

Schema::create('two', function ($table) {
$table->increments('id');
$table->integer('one_id');
$table->integer('quantity');
});

Schema::create('three', function ($table) {
$table->increments('id');
});

Schema::create('four', function ($table) {
$table->increments('id');
$table->integer('three_id');
$table->integer('quantity');
});
}

public function testSingleColumn()
{
$one = Model1::create();
$one->twos()->createMany([
['quantity' => 3],
['quantity' => 4],
['quantity' => 2],
]);

$result = Model1::withSum('twos:quantity')->first();

$this->assertEquals(9, $result->twos_quantity_sum);
}

public function testMultipleColumns()
{
$one = Model1::create();
$one->twos()->createMany([
['quantity' => 8],
['quantity' => 1],
]);

$result = Model1::withMax('twos:quantity', 'twos:id')->first();

$this->assertEquals(8, $result->twos_quantity_max);
$this->assertEquals(2, $result->twos_id_max);
}

public function testWithConstraintsAndAlias()
{
$one = Model1::create();
$one->twos()->createMany([
['quantity' => 3],
['quantity' => 1],
['quantity' => 0],
['quantity' => 5],
['quantity' => 1],
]);

$result = Model1::withAvg(['twos:quantity as avg_quantity' => function ($q) {
$q->where('quantity', '>', 2);
}])->first();

$this->assertEquals(4, $result->avg_quantity);
}

public function testAttributeEagerLoading()
{
$three = Model3::create();
$three->fours()->createMany([
['quantity' => 5],
['quantity' => 3],
['quantity' => 1],
]);

$result = Model3::first();

$this->assertEquals([
'id' => 1,
'fours_quantity_avg' => 3,
'fours_quantity_max' => 5,
'fours_id_max' => 3,
], $result->toArray());
}
}

class Model1 extends Model
{
public $table = 'one';
public $timestamps = false;
protected $guarded = ['id'];

public function twos()
{
return $this->hasMany(Model2::class, 'one_id');
}
}

class Model2 extends Model
{
public $table = 'two';
public $timestamps = false;
protected $guarded = ['id'];
}

class Model3 extends Model
{
public $table = 'three';
public $timestamps = false;
protected $guarded = ['id'];
protected $withAggregates = [
'avg' => 'fours:quantity',
'max' => [
'fours:quantity',
'fours:id',
],
];

public function fours()
{
return $this->hasMany(Model4::class, 'three_id');
}
}

class Model4 extends Model
{
public $table = 'four';
public $timestamps = false;
protected $guarded = ['id'];
}