laravel-query-builder icon indicating copy to clipboard operation
laravel-query-builder copied to clipboard

Support one whereHas for several filters.

Open IndexZer0 opened this issue 1 year ago • 2 comments

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

https://github.com/spatie/laravel-query-builder/discussions/634

https://github.com/spatie/laravel-query-builder/discussions/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 avatar Apr 07 '24 00:04 IndexZer0

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?

AlexVanderbist avatar May 10 '24 09:05 AlexVanderbist

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 :)

IndexZer0 avatar May 19 '24 12:05 IndexZer0

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

spatie-bot avatar Sep 20 '24 10:09 spatie-bot