Laravel-json-api: Filter by Model Scopes

[shrink]

Hi! First, I love this package, it's a pleasure to work with, thank you very much. I am building out a service to replace the backend of our system that currently supports hundreds of millions of page views per month and laravel-json-api is an integral (and enjoyable) part of this effort. From documentation to testing, it's fantastic. Thank you!

During implementation in my project it became apparent that because my project is primarily powered by Eloquent Models that there is a lot of opportunity to leverage Query Scopes for filtering. I think it would be great if the default filtering approach in the Eloquent Adapter is based around query scopes.

Here's an example of how I am using a FiltersModels trait:

trait FiltersModels
{
    /**
     * Allowed filtering parameters with a scope, e.g:
     * ['authors' => 'byAuthors'].
     *
     * @var array[]
     */
    protected $filterScopes = [];

    /**
     * Apply query scopes based on filters included in the request.
     *
     * @param \Illuminate\Database\Eloquent\Builder Builder $query
     * @param \Illuminate\Support\Collection $filters
     *
     * @return void
     */
    protected function filter($query, Collection $filters): void
    {
        $filters->reject(function ($value, $key) {
            return ! array_key_exists($key, $this->filterScopes);
        })->each(function ($value, $filter) use ($query) {
            $scope = $this->filterScopes[$filter];
            $query->$scope($value);
        });
    }
}

Example Validator:

<?php declare(strict_types=1);

// ...

class Validators extends AbstractValidators
{
    /**
     * The filter field names a client is allowed send.
     *
     * @var string[]|null The allowed fields, an empty array for none allowed,
     *                    or null to allow all fields.
     */
    protected $allowedFilteringParameters = [
        'authors',
    ];

    /**
     * Get query parameter validation rules.
     *
     * @return array
     */
    protected function queryRules(): array
    {
        return [
            'filter.authors' => 'filled|array',
            'filter.authors.*' => 'integer',
        ];
    }
}

Example Adapter:

<?php declare(strict_types=1);

// ...

class Adapter extends AbstractAdapter
{
    use FiltersModels;

    /**
     * Adapter constructor.
     *
     * @param StandardStrategy $paging
     */
    public function __construct(StandardStrategy $paging)
    {
        parent::__construct(new Comment, $paging);
    }

    /**
     * allowedFilteringParameters with an array pair containing the filter
     * parameter and model scope.
     *
     * @var array[]
     */
    protected $filterScopes = [
        'authors' => 'byAuthors',
    ];
}

Example Model:

<?php declare(strict_types=1);

// ...

class Comment extends Model
{
     /**
     * Scope query by authors.
     *
     * @param \Illuminate\Database\Eloquent\Builder $query
     * @param array $authors
     *
     * @return \Illuminate\Database\Eloquent\Builder
     */
    public function scopeByAuthors(Builder $query, array $authors): Builder
    {
        return $query->whereIn('author_id', $authors);
    }
}

To enable scope-based filtering for a resource I simply need to add a $filterScopes definition to the adapter and then validate the filter parameters in queryRules.

I think a basic implementation would involve replacing AbstractAdapter@filter with something like:

    protected $filterScopes = [];

    protected function filter($query, Collection $filters): void
    {
        $filters->reject(function ($value, $key) {
            return ! array_key_exists($key, $this->filterScopes);
        })->each(function ($value, $filter) use ($query) {
            $scope = $this->filterScopes[$filter];
            $query->$scope($value);
        });
    }

Notes:

1. Although by default all filters are allowed by the library, scopes would need to be explicitly defined so this does not enable a client to request a scope that isn't explicitly permitted by the developer.
2. I originally defined filtering scopes with a parameter type (e.g: ['authors' => ['byAuthors', 'array']) which then used settype on the value to enforce, however with a combination of typehinting and queryRules validation I do not think it is necessary, especially as it would prevent use of a scope that accepts multiple types -- which we could work around but that would introduce a lot of additional complexity so I'm leaning towards validation + typehints being enough security.
3. Because the functionality depends on the explicit definition of filterScopes it does not break backwards compatibility.
4. If a developer wishes to provide their own per-adapter filtering they can provide their own filter method as they do now (or extend the functionality using parent).

If this approach is suitable as the default filtering included in the Eloquent Adapter -- or as an optional trait -- I'm happy to submit a Pull Request with an implementation (incl. tests and documentation) -- wanted to check if this was desirable first as I may have missed a consideration.

Thanks!

[lindyhopchris]

Hi! Thanks for your feedback - really great to hear the package is useful!

This fits with what we generally do - i.e. we tend to put a lot of the filtering logic in the model, so that it can be used in other (non-JSON API) scenarios too.

I am however very reluctant to put a default filter method on the Eloquent adapter - because the JSON API spec is totally agnostic about how filtering is implemented, so I don't think this package should default an approach. However what you propose fits nicely with how Eloquent models work, so I'd accept a PR for it being an optional trait. We could obviously adjust this thinking in the future for a major release, but let's start with an opt-in trait.

There is however a complexity with this. The $query argument in the filter method can be an Eloquent query builder or an Eloquent relation (I think... will need to check that). However, as shown in your example code it is typical for people to type-hint the query builder itself in their scopes (and that's the Laravel pattern we'd need to stick to). So your trait will need to handle that somehow - I think that's possible so hopefully won't be a stumbling block.

In terms of your specific notes:

1. Yes, I think the developer should always define the allowed keys in the filter query parameter - i.e. at the validation stage the request should be rejected if an unrecognised key is received from the client. I think for 2.0 I'm probably going to switch the package to always expecting the developer to specify what keys are allowed by default anyway, as this is a lot better approach for an API IMHO.

2. Query values should always be validated, so the adapter should not have to worry about what value it is passing through to the scope. As the adapter filter method is always hit after validation, the responsibility lies with the developer to define acceptable validation rules. Laravel is not strongly type hinted, particularly in the query builder, i.e. passing 1 and '1' and that's why validation rules such as integer don't actually enforce it being a real PHP integer.

3. Making it an opt-in trait (especially for 1.x) avoids any unintentional breaking changes.

4. Same, making this opt-in is the best approach at the mo I think.

For consistency with the package, my thoughts are that the trait should look something like this as a starting point (we'll need to deal with the relation issue mentioned above):

<?php

namespace CloudCreativity\LaravelJsonApi\Eloquent\Concerns;

use CloudCreativity\LaravelJsonApi\Utils\Str;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Support\Collection;

trait FiltersModels
{

    /**
     * Mapping of filter keys to model query scopes.
     *
     * This trait will map JSON API filters to model scopes, and pass
     * the filter value to that scope. For example, if the client has
     * sent a `filter[authors]` query parameter, this trait expects
     * there to be an `authors` scope on the model.
     *
     * If you need to map a filter parameter to a different scope name,
     * then you can define it here. For example if `filter[authors]`
     * needed to be passed to the `byAuthors` scope, it can be defined
     * as follows:
     *
     * ```php
     * protected $filterScopes = [
 *      'authors' => 'byAuthors'
     * ];
     * ```
     *
     * If you want to a filter parameter to not be mapped to a scope,
     * define the mapping as `null`, for example:
     *
     * ```php
     * protected $filterScopes = [
     *      'authors' => null
     * ];
     * ```
     *
     * @var array
     */
    protected $filterScopes = [];

    /**
     * Apply the supplied filters to the builder instance.
     *
     * @param Builder $query
     * @param Collection $filters
     * @return void
     */
    protected function filter($query, Collection $filters)
    {
        $this->filterWithScopes($query, $filters);
    }

    /**
     * @param $query
     * @param Collection $filters
     * @return void
     */
    protected function filterWithScopes($query, Collection $filters): void
    {
        foreach ($filters as $key => $value) {
            if ($scope = $this->modelScopeForFilter($key)) {
                $this->filterWithScope($query, $scope, $value);
            }
        }
    }

    /**
     * @param $query
     * @param string $scope
     * @param $value
     * @return void
     */
    protected function filterWithScope($query, string $scope, $value): void
    {
        $query->{$scope}($value);
    }

    /**
     * @param string $key
     * @return string|null
     */
    protected function modelScopeForFilter(string $key): ?string
    {
        if (array_key_exists($key, $this->filterScopes)) {
            return $this->filterScopes[$key];
        }

        return Str::camelize($key);
    }
}

The reason for chunking the functionality into separate methods is so that the developer can overload particular methods if needed.

This would also allow a developer to selectively use the scope functionality for some filters, but not others. For example, if the trait was applied to my adapter, I could then do this:

protected function filter($query, Collection $filters)
{
    $this->filterWithScopes($query, $filters->only('foo', 'bar', 'bat'));
    $this->applyBazFilter($query); // something custom with baz.
}

[shrink]

@lindyhopchris thank you very much for the comprehensive feedback, very helpful to learn your motivations. I agree that what you've outlined is a good approach.

I hadn't caught that filter could receive multiple types. My brief understanding is that Laravel Relations forward calls to the Builder which should allow scopes to be applied regardless of what filter receives. I'll investigate that today and report back with what I've learned!

[lindyhopchris]

@citricsquid yes that's right it forwards calls... but the problem if is the scopeByAuthors method has type-hinted an Eloquent builder, it will fail.

There's a getQuery method on the Relation class to get the query builder... presumably if you apply the filters to the instance that it returns, all will be ok... but haven't tried.

[lindyhopchris]

Actually, maybe Laravel just takes care of that itself... will have to see!

[shrink]

Type-hinting Builder works correctly with relations! No additional work there. I will submit a PR in the next couple of days for you to review.

[wojo1206]

This is great trait, allows to utilize Eloquent local scope, works great. Thank You.

[lindyhopchris]

@citricsquid just wanted to check, are you still planning to do a PR or should I take care of getting this feature added?

[shrink]

@lindyhopchris apologies for the delay, busy couple of weeks. I have some free time this weekend so I can get it pushed this weekend if that works for you, otherwise you're welcome to go ahead with it if you'd like to. Thanks!

Edit: may take a couple more days, had much less time this weekend that anticipated.

[lindyhopchris]

@citricsquid no problem, i understand finding free time as I can never find enough open source time to tackle all the issues! I leave this for a bit to see if you have time as that means i can look at other issues.

[lindyhopchris]

This is now on the develop branch, refer to these docs for instructions:
https://github.com/cloudcreativity/laravel-json-api/blob/develop/docs/fetching/filtering.md

Basically the trait is applied to the Eloquent adapter - so it's on every adapter. However it is opt-in, so that filterWithScopes() must be called within the filter() method to use it. I've updated the stub for the adapter so when one is generated, it has the call in there.

Interesting this also works with Eloquent's magic where* method. So what I've implemented checking for a scope, and if that doesn't exist it falls back to e.g. whereSlug() for the slug filter.

Would be good if people watching this issue could give it a go. Set your minimum stability to dev then use composer require cloudcreativity/laravel-json-api:^1.4.

[lindyhopchris]

This was released as 1.4.0