Framework: Unnecessary ORDER BY when getting relationship count

[KKSzymanowski]

• Laravel Version: 5.8.7, 6.0.0
• PHP Version: 7.1.13, 7.3.9
• Database Driver & Version: MySQL 5.5.5-10.3.8-MariaDB

Description:

If a model has default sorting order, for example:

class Product extends Model
{
    public static function boot()
    {
        parent::boot();

        static::addGlobalScope(function($query) {
            $query->latest();
        });
    }
}

the regular count query is generated correctly:

$user->products()->count()

produces

select 
    count(*) as aggregate 
from `products` 
where `products`.`user_id` = ? and `products`.`user_id` is not null

However when getting users with their product count:

User::withCount('products')->first();

Laravel sends the following query:

select 
    `users`.*, 
    (
        select 
            count(*) 
        from `products` 
        where `users`.`id` = `products`.`user_id` 
        order by `created_at` desc
    ) as `products_count` 
from `users` 
limit 1

The order by clause in the last SQL is at the very least unnecessary and in some cases it generates a MySQL error:

SQLSTATE[42000]: Syntax error or access violation: 1140 
Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause 
(SQL: select `users`.*, (select count(*) from `products` where `users`.`id` = `products`.`user_id` order by `created_at` desc) as `products_count` from `users` limit 1)

Steps To Reproduce:

With a fresh Laravel 6 application:

1. Create a new Product model with the following migration:

$table->bigIncrements('id');
$table->integer('user_id');
$table->timestamps();

1. Add the default sorting order to the Product model:

public static function boot()
{
    parent::boot();

    static::addGlobalScope(function($query) {
        $query->latest();
    });
}

1. Add the relationship definition to the User model

public function products()
{
    return $this->hasMany(Product::class);
}

1. Launch the Tinker and run:

DB::listen(function($q) {echo $q->sql . "\n";});
User::withCount('products')->first()

You should at least see the order by clause in the generated query and in some cases you will see the MySQL syntax error.

[driesvints]

So what you're saying is that for count methods any global scope shouldn't be applied? Because from your code I actually expect this to happen. I'm not sure it's wanted to ignore global scopes for the count method.

[KKSzymanowski]

No, global scopes should be applied globally, even for count methods, but ordering doesn't make sense when getting just the row count.

When fetching just the relationship count by using the scopes are applied correctly.
Let's say I have the following global scope:

static::addGlobalScope(function($query) {
    $query->where('id', '>', 15)->latest();
});

The User::first()->products()->count() call produces the following query:

select 
    count(*) as aggregate 
from `products` 
where `products`.`user_id` = ? 
    and `products`.`user_id` is not null 
    and `id` > ?

with the value 15 bound to the last ? parameter. Note that the WHERE clause is present but the ORDER BY clause isn't.

However this doesn't work the same when fetching a row with some relationship count.
The User::withCount('products')->first() call produces the following query:

select 
    `users`.*, 
    (
        select 
            count(*) 
        from `products` 
        where `users`.`id` = `products`.`user_id` 
            and `id` > ? 
        order by `created_at` desc
    ) as `products_count` 
from `users` 
limit 1

once again with the value 15 bound to the parameter. Note here that both the WHERE and ORDER BY clauses are present in the sub-select.

[driesvints]

@KKSzymanowski I know what you're saying but that's exactly the point I'm trying to make. You're explicitly setting a global scope of latest and thus applying the order by to every query. Imo Eloquent shouldn't magically remove that (even for count queries).

[KKSzymanowski]

Then why is the ORDER BY removed from the query when doing User::first()->products()->count()?

[driesvints]

Hmm, that I don't know. I can't seem to find where that happens.

[KKSzymanowski]

Illuminate\Database\Query\Builder::setAggregate

[KKSzymanowski]

One way to approach this would be to change Illuminate\Database\Eloquent\Relations\Relation::getRelationExistenceQuery() to include the call to Illuminate\Database\Query\Builder::setAggregate().

This is tricky however because when setAggregate is called on the Eloquent\Builder the deletion of orders and bindings['order'] doesn't affect the final query because the global scope is added to the builder when it's being cast to Query\Builder:

public function toBase()
{
    return $this->applyScopes()->getQuery();
}

You would have to cast it to Query\Builder, call setAggregate() and wrap it back in an Eloquent\Builder to return it back to Illuminate\Database\Eloquent\Concerns\QueriesRelationships::withCount().

This is a proof of concept which works in my case:
Illuminate\Database\Eloquent\Relations\Relation:

public function getRelationExistenceQuery(Builder $query, Builder $parentQuery, $columns = ['*'])
{
-   return $query->select($columns)->whereColumn(
-       $this->getQualifiedParentKeyName(), '=', $this->getExistenceCompareKey()
-   );
+   return new Builder(
+       $query->whereColumn(
+           $this->getQualifiedParentKeyName(), '=', $this->getExistenceCompareKey()
+       )->toBase()->setAggregate('count', ['*'])
+   );
}

Illuminate\Database\Eloquent\Concerns\QueriesRelationships

public function withCount($relations)
{
    if (empty($relations)) {
        return $this;
    }

    if (is_null($this->query->columns)) {
        $this->query->select([$this->query->from.'.*']);
    }

    $relations = is_array($relations) ? $relations : func_get_args();

    foreach ($this->parseWithRelations($relations) 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.
        $segments = explode(' ', $name);

        unset($alias);

        if (count($segments) === 3 && Str::lower($segments[1]) === 'as') {
            [$name, $alias] = [$segments[0], $segments[2]];
        }

        $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
        );

        $query->callScope($constraints);

        $query = $query->mergeConstraintsFrom($relation->getQuery())->toBase();

-       if (count($query->columns) > 1) {
+       if ($query->columns !== null && count($query->columns) > 1) {
            $query->columns = [$query->columns[0]];
        }

        // 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');

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

    return $this;
}

The added null check is required because without the select() call in getRelationExistenceQuery the EloquentBuilder::columns is null.

With the above changes the produced SQL is:

select 
    `users`.*, 
    (
        select 
            count(*) as aggregate 
        from `products` 
        where `users`.`id` = `products`.`user_id` 
            and `id` > ?
    ) as `products_count` 
from `users` 
limit 1

[KKSzymanowski]

A workaround for now would be to check if addGlobalScope() has been called by the withCount() method:

public static function boot()
{
    parent::boot();

    static::addGlobalScope(function($query) {
        $query->where('id', '>', 15);

        $backtrace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS);
        foreach ($backtrace as $call) {
            if($call['function'] == 'withCount' && $call['class'] == \Illuminate\Database\Eloquent\Builder::class) {
                return;
            }
        }

        $query->latest();
    });
}

The performance impact from the debug_backtrace() call and iterating over its result doesn't seem to be very high.

[staudenmeir]

I agree that this should be fixed (if possible).

19022 fixed the issue for "normal" aggregate queries.

[driesvints]

Okay. Feel free to send in a PR if you can figure something out.

[KKSzymanowski]

Is the approach I provided previously correct?

[driesvints]

@KKSzymanowski it looks okay to me. @staudenmeir what do you think?

Maybe it's best though that this is sent to the master branch.

[staudenmeir]

Your approach doesn't work like that, it removes all global scopes from the query. Also, we need to adjust getRelationExistenceCountQuery(), not getRelationExistenceQuery().

We don't need to call setAggregate() (it's protected), we can just copy the relevant code.

[nmokkenstorm]

I took the liberty of adding a PR for this, as I saw this issue and happened to have been digging through the Eloquent relationship code recently. As I mention in the PR, the solution seems so be too straightforward to be true, but it seems to do the trick.

@staudenmeir modifying getRelationExistenceCountQuery() would also work. A quick grep through the codebase shows that the only place it is used is in the QueriesRelationships trait, but it'd break the immediate return.

[staudenmeir]

This has been fixed.

[KKSzymanowski]

Thank you!