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

Support one whereHas for several filters. #931

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

IndexZer0
Copy link

@IndexZer0 IndexZer0 commented Apr 7, 2024

This PR adds AllowedRelationshipFilter class to be used when wanting to group multiple AllowedFilter's into the same exists query.

Problem:

When using multiple AllowedFilter's for nested relations - the package adds mutliple exist clauses to the query.

->allowedFilters([
    AllowedFilter::exact('relatedModels.id'),
    AllowedFilter::exact('relatedModels.name'),
    AllowedFilter::exact('relatedModels.nestedRelatedModels.id'),
    AllowedFilter::exact('relatedModels.nestedRelatedModels.name'),
])
  • Generates 6 EXISTS queries.
SELECT
    *
FROM
    `test_models`
WHERE
    EXISTS (
        SELECT
            *
        FROM
            `related_models`
        WHERE
            `test_models`.`id` = `related_models`.`test_model_id`
            AND `related_models`.`id` = 1)
        AND EXISTS (
            SELECT
                *
            FROM
                `related_models`
            WHERE
                `test_models`.`id` = `related_models`.`test_model_id`
                AND `related_models`.`name` = 'asdf')
            AND EXISTS (
                SELECT
                    *
                FROM
                    `related_models`
                WHERE
                    `test_models`.`id` = `related_models`.`test_model_id`
                    AND EXISTS (
                        SELECT
                            *
                        FROM
                            `nested_related_models`
                        WHERE
                            `related_models`.`id` = `nested_related_models`.`related_model_id`
                            AND `nested_related_models`.`id` = 1))
                    AND EXISTS (
                        SELECT
                            *
                        FROM
                            `related_models`
                        WHERE
                            `test_models`.`id` = `related_models`.`test_model_id`
                            AND EXISTS (
                                SELECT
                                    *
                                FROM
                                    `nested_related_models`
                                WHERE
                                    `related_models`.`id` = `nested_related_models`.`related_model_id`
                                    AND `nested_related_models`.`name` = 'ghjk'))

Desired Query

  • 2 EXISTS queries.
SELECT
    *
FROM
    `test_models`
WHERE
    EXISTS (
        SELECT
            *
        FROM
            `related_models`
        WHERE
            `test_models`.`id` = `related_models`.`test_model_id`
            AND `related_models`.`id` = 1
            AND `related_models`.`name` = 'asdf'
            AND EXISTS (
                SELECT
                    *
                FROM
                    `nested_related_models`
                WHERE
                    `related_models`.`id` = `nested_related_models`.`related_model_id`
                    AND `nested_related_models`.`id` = 1
                    AND `nested_related_models`.`name` = 'ghjk'))

Usage

->allowedFilters([
    AllowedRelationshipFilter::group('relatedModels', ...[
        AllowedFilter::exact('relatedModels.id', 'id'),
        AllowedFilter::exact('relatedModels.name', 'name'),
        AllowedRelationshipFilter::group('nestedRelatedModels', ...[
            AllowedFilter::exact('relatedModels.nestedRelatedModels.id', 'id'),
            AllowedFilter::exact('relatedModels.nestedRelatedModels.name', 'name'),
        ]),
    ]),
]);

I have seen various requests for this in the past, for example

#634

#663


  • Tests have been added to demonstrate useage.
  • Documentation not updated.
    • Will update once preliminary go ahead for this feature has been confirmed.

At this point just looking for your thoughts and feedback on this.

To me, this feels like quite an important missing feature of this package and if the core maintainers could give some feedback, insight and pointers on this then I'm happy to make any changes and then update documentation to match.

IndexZer0 and others added 5 commits April 7, 2024 00:32
- Modify FiltersQuery to support AllowedRelationshipFilter.
- Modify AllowedFilter to support AllowedRelationshipFilter.
- Added tests for different exist clauses cases.
$this->allowedFilters->each(
function (AllowedFilterContract $allowedFilter) use ($query, $value) {
$allowedFilter->filter(
QueryBuilder::for($query),
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but why not pass the current $query object? It should already be an instance of QueryBuilder

Copy link
Author

Choose a reason for hiding this comment

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

Tried to swap to $query and get error.

Spatie\QueryBuilder\AllowedFilter::filter(): Argument #1 ($query) must be of type Spatie\QueryBuilder\QueryBuilder, Illuminate\Database\Eloquent\Builder given, called in /src/AllowedRelationshipFilter.php on line 27

Keeping this as is.

Comment on lines +204 to +212
public function isRequested(QueryBuilderRequest $request): bool
{
return $request->filters()->has($this->getName());
}

public function getValueFromRequest(QueryBuilderRequest $request): mixed
{
return $request->filters()->get($this->getName());
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice little refactor 👍

@AlexVanderbist
Copy link
Member

Hey @IndexZer0, thanks for the extensive research and PR! As the impact on the existing filters is minimal, I'm happy to merge this in its current state in the v6 release and see what feedback comes in once people start using the AllowedRelationshipFilter::group() method.

Is there anything specific you would like feedback on?

@IndexZer0 IndexZer0 changed the base branch from main to v5 May 19, 2024 11:59
@IndexZer0 IndexZer0 changed the base branch from v5 to main May 19, 2024 11:59
@IndexZer0
Copy link
Author

Hey @IndexZer0, thanks for the extensive research and PR! As the impact on the existing filters is minimal, I'm happy to merge this in its current state in the v6 release and see what feedback comes in once people start using the AllowedRelationshipFilter::group() method.

Is there anything specific you would like feedback on?

Thanks for reviewing @AlexVanderbist

Not particulary after specific feedback if you're happy to merge this feature. Just was open to changing the implementation approach if you had any better ideas on how to structure this.

  • Updated to this fork to current spatie:main commit.
  • Fixed merge conflicts.
  • Added brief example to documentation.
  • My github actions show tests are failing.
    • Though these tests are failing in spatie:main too.
    • The newly added tests in this PR are passing currently.

FYI - As it turns out, this package is not going to suit my use case anymore so doubt I'd have the desire to update this PR again.

Hope you can get this merged soon before more merge conflicts :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants