staudenmeir / eloquent-eager-limit

Laravel Eloquent eager loading with limit
MIT License
880 stars 62 forks source link

Eloquent Eager Limiting Doesn't Work Inside Scopes #30

Closed moliver91 closed 4 years ago

moliver91 commented 4 years ago

There appears to be a limitation which prevents the package from intercepting limits set in global scopes. I was hoping to be able to use a global scope to clean up some of my API's controllers.

For instance:

return new AreaCollection(Area::on($database)->where($request->filter['areas'])->limit($request['areas'] ?? 100)->get());

Becomes:


// Controller method

return new AreaCollect(Area::on($database)->get());

// GlobalScopes

static::addGlobalScope('filters', function (Builder $builder) {
    $builder->where(request()->filter[Str::plural(strtolower(class_basename(static::class)))] ?? []);
});

static::addGlobalScope('limit', function (Builder $builder) {
    $builder->limit(request()->limit[Str::plural(strtolower(class_basename(static::class)))] ?? 100);
});

This allows me to clean up my controllers and enforce behavior across all of my resources by including these as traits. It would be nice if these global scopes would utilize eager limiting, but it seems that they perform the default behavior unless I limit directly in the controller rather than the global scopes.

I'll spend some more time looking at this tonight, but I figure someone on this project is probably a much more experienced programmer and will know immediately why this works this way.

staudenmeir commented 4 years ago

What queries are executed in these two cases?

moliver91 commented 4 years ago

I apologize, the examples above are poor examples of the problem. Here is a much better example along with the requested info:

Without Global Scopes:

new AreaCollection(Area::on($database)->with(['arrivals' => function ($query) {
    $query->take(request()->limit['arrivals'] ?? 100);
}])->where($request->filter['areas'] ?? [])->limit($request->limit['areas'] ?? 100)->get());

SQL:

select top 100 * from [Areas] order by [Description] asc

select * from (select [GroupArrivals].*, [GroupAreaBookings].[AreaGUID] as [pivot_AreaGUID], [GroupAreaBookings].[RefID] as [pivot_RefID], [GroupAreaBookings].[StartDateTime] as [pivot_StartDateTime], [GroupAreaBookings].[EndDateTime] as [pivot_EndDateTime], row_number() over (partition by [GroupAreaBookings].[AreaGUID] order by [TimeCreated] desc) as laravel_row from [GroupArrivals] inner join [GroupAreaBookings] on [GroupArrivals].[RefID] = [GroupAreaBookings].[RefID] where [GroupAreaBookings].[AreaGUID] in (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)) as laravel_table where laravel_row <= 1 order by laravel_row

With Global Scopes:

new AreaCollection(Area::on($database)->with('arrivals')->get());

SQL:

select top 100 * from [Areas] order by [Description] asc

select top 1 [GroupArrivals].*, [GroupAreaBookings].[AreaGUID] as [pivot_AreaGUID], [GroupAreaBookings].[RefID] as [pivot_RefID], [GroupAreaBookings].[StartDateTime] as [pivot_StartDateTime], [GroupAreaBookings].[EndDateTime] as [pivot_EndDateTime] from [GroupArrivals] inner join [GroupAreaBookings] on [GroupArrivals].[RefID] = [GroupAreaBookings].[RefID] where [GroupAreaBookings].[AreaGUID] in (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) order by [TimeCreated] desc
moliver91 commented 4 years ago

Looking through Laravel's base Model class, it's clear to me that the issue lies somewhere in the fact that registerGlobalScopes() calls newQueryWithoutScopes which calls newModelQuery() which provides an instance of Illuminate\Database\Eloquent\Builder (through newEloquentBuilder()) which this package doesn't touch, I think.

staudenmeir commented 4 years ago

Can you share the Area and Arrival model?

moliver91 commented 4 years ago

Area:

class Area extends ApiModel
{
    protected $table = 'Areas';
    protected $primaryKey = 'AreaGUID';
    public $incrementing = false;
    protected $keyType = 'string';
    public $timestamps = false;

    protected static function booted()
    {
        static::addGlobalScope('order', function (Builder $builder) {
            $builder->orderBy('Description', 'asc');
        });

    }

    public function arrivals()
    {
        return $this->belongsToMany('MarshallOliver\LaravelCenterEdgeAPI\Arrival', 'GroupAreaBookings', 'AreaGUID', 'RefID')
                    ->withPivot('AreaGUID', 'StartDateTime', 'EndDateTime');
    }

}

Arrival:

class Arrival extends ApiModel
{
    protected $table = 'GroupArrivals';
    protected $primaryKey = 'RefID';
    public $incrementing = false;
    protected $keyType = 'string';
    public $timestamps = false;

    protected $hidden = ['SecurityCode'];

    protected static function booted()
    {
        static::addGlobalScope('order', function (Builder $builder) {
            $builder->orderBy('TimeCreated', 'desc');
        });
    }

    public function areas()
    {
        return $this->belongsToMany('MarshallOliver\LaravelCenterEdgeAPI\Area', 'GroupAreaBookings', 'RefID', 'AreaGUID')
                    ->withPivot('RefID', 'StartDateTime', 'EndDateTime');
    }

}

ApiModel:

class ApiModel extends Model
{

    use HasEagerLimit;
    use Limitable;
    use Filterable;

}

Limitable:

trait Limitable
{

    public static function bootLimitable()
    {

        static::addGlobalScope('limit', function (Builder $builder) {
            $builder->limit(request()->limit[Str::plural(strtolower(class_basename(static::class)))] ?? 100);
        });

    }

}
staudenmeir commented 4 years ago

Unfortunately, it's not possible to use global scopes like this. The limit needs to be applied directly to the relationship (not the query builder). This would even be the case for a native implementation of this package in Laravel.

Looking through Laravel's base Model class, it's clear to me that the issue lies somewhere in the fact that registerGlobalScopes() calls newQueryWithoutScopes which calls newModelQuery() which provides an instance of Illuminate\Database\Eloquent\Builder (through newEloquentBuilder()) which this package doesn't touch, I think.

The package doesn't override newEloquentBuilder() but the underlying newBaseQueryBuilder().

moliver91 commented 4 years ago

I spent a lot of time looking at this today and I agree. Due to the point at which global scopes are added within Laravel's Model class, there is no way to discern if the query currently being built is for a relation or not so it would not even be possible to, for example, create a Global Scope that only applies to a model when being referenced as a relation.

The solution that I've come up with is, I think, the best I've got for now and I'll document it here in case anyone else comes along wanting to use this package alongside global scopes.

To clarify, this utilizes the package very well. The only downside is that I have to essentially create two methods for every relation on a model, but c'est la vie.

// Resource Model

class Area extends ApiModel
{
    protected $table = 'Areas';
    protected $primaryKey = 'AreaGUID';
    public $incrementing = false;
    protected $keyType = 'string';
    public $timestamps = false;

    protected static function booted()
    {
        static::addGlobalScope('order', function (Builder $builder) {
            $builder->orderBy('Description', 'asc');
        });

    }

    public function arrivals()
    {
        return $this->belongsToMany('MarshallOliver\LaravelCenterEdgeAPI\Arrival', 'GroupAreaBookings', 'AreaGUID', 'RefID')
                    ->withPivot('AreaGUID', 'StartDateTime', 'EndDateTime');
    }

    public function scopeWithLimitedArrivals($query)
    {
        return $query->with(['arrivals' => function ($query) {
            $query->withoutGlobalScope('limits')->limit(request()->limit['arrivals'] ?? 100);
        }]);
    }

}

// Resource Controller

    public function areasWithArrivals($database, Request $request)
    {

        return new AreaCollection(Area::on($database)->withLimitedArrivals()->get());

    }

Thanks for taking a look at this!