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.

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

PHPORM-238 Add support for withCount and other aggregations #3182

Open
wants to merge 11 commits into
base: 5.x
Choose a base branch
from
93 changes: 93 additions & 0 deletions src/Eloquent/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
use Illuminate\Database\Eloquent\Builder as EloquentBuilder;
use Illuminate\Database\Eloquent\Collection;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\Relation;
use Illuminate\Support\Str;
use InvalidArgumentException;
use MongoDB\BSON\Document;
use MongoDB\Builder\Type\QueryInterface;
use MongoDB\Builder\Type\SearchOperatorInterface;
Expand All @@ -15,15 +18,20 @@
use MongoDB\Laravel\Connection;
use MongoDB\Laravel\Helpers\QueriesRelationships;
use MongoDB\Laravel\Query\AggregationBuilder;
use MongoDB\Laravel\Relations\EmbedsOneOrMany;
use MongoDB\Laravel\Relations\HasMany;
use MongoDB\Model\BSONDocument;

use function array_key_exists;
use function array_merge;
use function collect;
use function count;
use function explode;
use function is_array;
use function is_object;
use function iterator_to_array;
use function property_exists;
use function sprintf;

/**
* @method \MongoDB\Laravel\Query\Builder toBase()
Expand All @@ -34,6 +42,9 @@
private const DUPLICATE_KEY_ERROR = 11000;
use QueriesRelationships;

/** @var array{relation: Relation, function: string, constraints: array, column: string, alias: string}[] */
private array $withAggregate = [];
jmikola marked this conversation as resolved.
Show resolved Hide resolved

/**
* The methods that should be returned from query builder.
*
Expand Down Expand Up @@ -294,6 +305,88 @@
}
}

public function withAggregate($relations, $column, $function = null)
jmikola marked this conversation as resolved.
Show resolved Hide resolved
{
if (empty($relations)) {
return $this;
}

$relations = is_array($relations) ? $relations : [$relations];

foreach ($this->parseWithRelations($relations) as $name => $constraints) {
Copy link
Member

Choose a reason for hiding this comment

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

I assume this calls this method on the base class but it's not clear what "parse a list of relations into individuals" means for the structure of the return value, which is only described as an array. In particular, it's not clear how "<field> as <alias>" would be returned.

Would a $name with exactly three space-delimited segments always have that structure?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's adding the constraint closure from the scope and the relationships.

// For "count" and "exist" we can use the embedded list of ids
// for embedded relations, everything can be computed directly using a projection.
$segments = explode(' ', $name);

$name = $segments[0];
$alias = (count($segments) === 3 && Str::lower($segments[1]) === 'as' ? $segments[2] : Str::snake($name) . '_' . $function);
jmikola marked this conversation as resolved.
Show resolved Hide resolved

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

if ($relation instanceof EmbedsOneOrMany) {
switch ($function) {
case 'count':
$this->project([$alias => ['$size' => ['$ifNull' => ['$' . $name, []]]]]);
Fixed Show fixed Hide fixed
break;
case 'min':
case 'max':
case 'avg':
$this->project([$alias => ['$' . $function => '$' . $name . '.' . $column]]);
Fixed Show fixed Hide fixed
break;
default:
throw new InvalidArgumentException(sprintf('Invalid aggregate function "%s"', $function));
}
} else {
// @todo support "exists"
$this->withAggregate[$alias] = [
GromNaN marked this conversation as resolved.
Show resolved Hide resolved
'relation' => $relation,
'function' => $function,
'constraints' => $constraints,
'column' => $column,
'alias' => $alias,
];
}

// @todo HasMany ?

// Otherwise, we need to store the aggregate request to run during "eagerLoadRelation"
// after the root results are retrieved.
}

return $this;
}

public function eagerLoadRelations(array $models)
{
if ($this->withAggregate) {
$modelIds = collect($models)->pluck($this->model->getKeyName())->all();

foreach ($this->withAggregate as $withAggregate) {
if ($withAggregate['relation'] instanceof HasMany) {
Copy link
Member

Choose a reason for hiding this comment

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

Given that this only applies some processing for HasMany relations, under what circumstances would different relations get appended to $this->withAggregate? And are they OK being left as-is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding an exception in the "else" case. I thing the only other case if "HasOne", but I need to check "BelongsToMany".

$results = $withAggregate['relation']->newQuery()
->where($withAggregate['constraints'])
->whereIn($withAggregate['relation']->getForeignKeyName(), $modelIds)
->groupBy($withAggregate['relation']->getForeignKeyName())
->aggregate($withAggregate['function'], [$withAggregate['column']]);

foreach ($models as $model) {
$value = $withAggregate['function'] === 'count' ? 0 : null;
foreach ($results as $result) {
if ($model->getKey() === $result->{$withAggregate['relation']->getForeignKeyName()}) {
$value = $result->aggregate;
jmikola marked this conversation as resolved.
Show resolved Hide resolved
break;
}
}

$model->setAttribute($withAggregate['alias'], $value);
}
}
}
}

return parent::eagerLoadRelations($models);
}

/**
* Add the "updated at" column to an array of values.
* TODO Remove if https://github.com/laravel/framework/commit/6484744326531829341e1ff886cc9b628b20d73e
Expand Down
12 changes: 10 additions & 2 deletions src/Query/Builder.php
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ public function toMql(): array

$aggregations = blank($this->aggregate['columns']) ? [] : $this->aggregate['columns'];

if ($column === '*' && $function === 'count' && ! $this->groups) {
if (in_array('*', $aggregations) && $function === 'count' && empty($group['_id'])) {
$options = $this->inheritConnectionOptions($this->options);

return ['countDocuments' => [$wheres, $options]];
Expand Down Expand Up @@ -611,7 +611,7 @@ public function aggregate($function = null, $columns = ['*'])

$this->bindings['select'] = [];

$results = $this->get($columns);
$results = $this->get();

// Once we have executed the query, we will reset the aggregate property so
// that more select queries can be executed against the database without
Expand Down Expand Up @@ -650,6 +650,14 @@ public function aggregateByGroup(string $function, array $columns = ['*'])
return $this->aggregate($function, $columns);
}

public function count($columns = '*')
{
// Can be removed when available in Laravel: https://github.com/laravel/framework/pull/53209
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this PR was merged, then reverted, and then superseded by laravel/framework#53679. Should this be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then reverted again: laravel/framework#54196

Copy link
Member

Choose a reason for hiding this comment

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

In either case, the reference to laravel/framework#53209 looks outdated. Please adjust accordingly and then feel free to resolve this thread.

$results = $this->aggregate(__FUNCTION__, Arr::wrap($columns));

return $results instanceof Collection ? $results : (int) $results;
}

/** @inheritdoc */
public function exists()
{
Expand Down
Loading
Loading