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

[11.x] Add dynamic "where(Any|All|None)*" clauses to the query builder #52272

Closed

Conversation

einar-hansen
Copy link
Contributor

This PR introduces a new feature to the Laravel Query Builder that allows for dynamic "where(Any|All|None)*" clauses that will call a query on multiple columns. The idea of this came from the comments in PR#52147 and from the comments on yesterdays Whats new in Laravel video.

Key features:

  1. Dynamic method handling for "where(Any|All|None)*" clauses
  2. Support for "or" prefixed clauses (e.g., orWhereAny*, orWhereAll*, orWhereNone*)
  3. It will only call on existing Laravel where clauses, for example whereLike and whereIn etc.

Implementation details:

  • Added a new method dynamicWhereColumns to handle the dynamic where clauses. The implementation is inspired by the dynamicWhere method in the __call magic method.
  • Used regex pattern matching to identify and parse the dynamic method calls.
  • Implemented nested where clauses to group conditions appropriately.
  • Because of the way it is configured it allows you to call all of the where and orWhere clause types (e.g., whereBetween, whereIn, whereLike, whereDate, etc.). on multiple columns.
  • It will throw a BadMethodCallException if you try to call a non-existing method, like for example whereAllHelloWorld

Example usage:

->whereAnyLike(['first_name', 'last_name'], '%John%', true);
->whereAnyNotLike(['last_name', 'email'], '%Doe%');
->orWhereNoneLike(['first_name', 'last_name'], '%John%', true);

->whereAllBetween(['price', 'discounted_price'], [100, 200]);
->orWhereAllNotBetween(['price', 'discounted_price'], [100, 200]);

->whereAnyDate(['created_at', 'updated_at'], '2024-07-25');
->whereAllIn(['id', 'parent_id'], 1, 2, 3);

Lets search for a user:

User::query()
    ->whereAllLike(['first_name', 'last_name'], '%John%')
    ->orWhereAnyDate(['created_at', 'updated_at'], '2024-07-25')
    ->get();

Translates to

select * 
from "users" 
where ("first_name" like '%John%' and "last_name" like '%John%') 
or (date("created_at") = '2024-07-25' or date("updated_at") = '2024-07-25');

The PR includes a comprehensive test suite covering various scenarios:

  • Basic usage of whereAll, whereAny, and whereNone
  • Combinations with other where clauses
  • Different operators (like, not like, between, in, etc.)
  • Date and column comparisons
  • Error handling for undefined methods

Considerations:

  • Uses the magic __call method, which may impact IDE support (and it is wizardry)
  • Introduces numerous new method possibilities (can be viewed as both an advantage and a potential drawback)
  • We might want to white-list the supported methods, but I like the flexibility that a developer can call all of the query methods that start with where or orWhere
  • We might get some weird query names if the user for example combines the whereNone and whereNot clauses, like whereNoneNotIn orWhereNoneNotLike.

Let me know what you think. Marking this as draft until I know how the PR#52260 goes.

@einar-hansen einar-hansen marked this pull request as draft July 25, 2024 14:56
@einar-hansen einar-hansen force-pushed the feature/whereAnyAllNone-dynamic branch 2 times, most recently from 7c97394 to 09cc149 Compare July 25, 2024 15:18
@driesvints
Copy link
Member

Make sure to mark the PR as ready if you need a review.

@johanrosenson
Copy link
Contributor

Should they not be added as proper methods instead of using a regex in __call?
Then they could be added to BuilderContract as well, using __call prevents them from being added to the interface, which feels like a shame since the interface was added just recently.

@einar-hansen
Copy link
Contributor Author

Should they not be added as proper methods instead of using a regex in __call? Then they could be added to BuilderContract as well, using __call prevents them from being added to the interface, which feels like a shame since the interface was added just recently.

I agree with you in principle, but I think it can get messy really fast if we do so, since we have 6 methods for every "base-query"; whereAny, whereAll, whereNone, orWhereAny, orWhereAll and orWhereNone.

By quickly screening at all the "one-column" queries in Builder that can could potentially draw advantage of this feature I found 26 methods (list at bottom). That could lead to 156 methods. But for sure not all of them would ever be used, like the double negative queries mentioned in the PR, like orWhereNoneNotIn etc. So it could probably be dropped down to half, or less, but it would still be a lot of methods.

For these reasons, I like the simplicity of having this super simple and small implementation that uses the magic __call method that just applies the query to all of the columns. Maybe an improvement could to add docblocks for all the possibilities, just like we have for the facades. Thoughts on this @johanrosenson ?

whereBetween
whereBetweenColumns
whereColumn
whereDate
whereDay
whereIn
whereIntegerInRaw
whereIntegerNotInRaw
whereJsonContains
whereJsonContainsKey
whereJsonDoesntContain
whereJsonDoesntContainKey
whereJsonDoesntOverlap
whereJsonLength
whereJsonOverlaps
whereLike
whereMonth
whereNot
whereNotBetween
whereNotBetweenColumns
whereNotIn
whereNotLike
whereNotNull
whereNull
whereTime
whereYear

@einar-hansen
Copy link
Contributor Author

einar-hansen commented Jul 26, 2024

I was also playing around with an idea of updating the original whereAll and whereAny methods to accept a callback as the second parameter, then the method could execute the callback on all the column name strings, looking something like this.

$builder->whereAll(['cost_price', 'selling_price'], function(Builder $query, string $column) : void {
    $query->whereBetween($column, [0, 100]);
});

But it would be very verbose, which is against the point of the whereAny and whereAll methods.

@johanrosenson
Copy link
Contributor

I agree with you in principle, but I think it can get messy really fast if we do so, since we have 6 methods for every "base-query"; whereAny, whereAll, whereNone, orWhereAny, orWhereAll and orWhereNone.

I see what you want to do now, I misunderstood what you were doing at first.
I agree with you, adding them all as individual methods is not realistic, your implementation is good.

I was also playing around with an idea of updating the original whereAll and whereAny methods to accept a callback as the second parameter, then the method could execute the callback on all the column name strings, looking something like this.

This idea sounds very elegant 🚀 Can it be implemented in a way that makes the original signature still valid?

@einar-hansen
Copy link
Contributor Author

einar-hansen commented Jul 26, 2024

This idea sounds very elegant 🚀 Can it be implemented in a way that makes the original signature still valid?

Sure, add a check for Closure. The docblock already expects mixed

/**
     * Add a "where" clause to the query for multiple columns with "and" conditions between them.
     *
     * @param  string[]  $columns
     * @param  mixed  $operator
     * @param  null  $callback
     * @param  mixed  $value
     * @param  string  $boolean
     * @return $this
     */
    public function whereAll($columns, $operator = null, $value = null, $boolean = 'and')
    {
        if ($operator instanceof Closure) {
            $this->whereNested(function ($query) use ($columns, $operator) {
                foreach ($columns as $column) {
                    $operator($query, $column);
                }
            }, $boolean);

            return $this;
        }

        // Below is unchanged
        [$value, $operator] = $this->prepareValueAndOperator(
            $value, $operator, func_num_args() === 2
        );

        $this->whereNested(function ($query) use ($columns, $operator, $value) {
            foreach ($columns as $column) {
                $query->where($column, $operator, $value, 'and');
            }
        }, $boolean);

        return $this;
    }

Existing tests, plus this new one would pass with these changes:

        $builder = $this->getBuilder();
        $builder->select('*')->from('users')->whereAll(['price', 'cost'], function($query, $column): void {
            $query->whereBetween($column, [0, 100]);
        });
        $this->assertSame('select * from "users" where ("price" between ? and ? and "cost" between ? and ?)', $builder->toSql());
        $this->assertEquals([0, 100, 0, 100], $builder->getBindings());

What I don't like
I don't like the part where we send the $column variable into the callback, and then require the user to immediately use it in the query inside the callback. It feels a bit weird to me, and I don't think it is very developer friendly. It also increases the chances of it being used/implemented wrong.

It would be ideal to be able to use something like in the example below, skipping the $column variable. But it would require a new Builder class for these queries, which introduces a lot of complexity and extra code to maintain. So I would say that it would not be worth it.

$builder->whereAll(['sales_price', 'cost_price'], fn($columnQuery) => $columnQuery->whereBetween([0, 100]));
$builder->whereAll(['first_name', 'last_name'], fn($columnQuery) => $columnQuery->whereLike('%einar%'));

Considering all of this, I think it is better with a little magic and solve it like suggested in the PR. It reads betters and is more intuitive/familiar for the developer as well.

User::query()
    ->whereAllLike(['first_name', 'last_name'], '%John%')
    ->orWhereAnyDate(['created_at', 'updated_at'], '2024-07-25')
    ->get();

@chu121su12
Copy link
Contributor

chu121su12 commented Jul 29, 2024

whereBetween whereBetweenColumns ...

@einar-hansen How about adding the @ method phpdoc instead?

@einar-hansen
Copy link
Contributor Author

whereBetween whereBetweenColumns ...

@einar-hansen How about adding the @ method phpdoc instead?

I've been sleeping a bit on this one, and I've decided to explore the path with a NestedBuilder that we could use to achieve signatures like this:

$builder->whereAll(['sales_price', 'cost_price'], fn(NestedBuilder $query) => $query->whereBetween([0, 100]));
$builder->orWhereAny(['first_name', 'last_name'], fn(NestedBuilder $query) => $query->whereLike('%einar%'));

I think this will be the cleanest and easiest solution.
Advantages:

  • No new methods in the IDE for the Builder class (already many methods there)
  • We don't need 6 methods (no duplication) for each where clause, as the whereAll, whereAny, whereNone, orWhereAll, orWhereAny and orWhereNone will share the same NestedBuilder class
  • Better and easier to maintain IDE support since there will be no magic methods and no @method do blocks to manually maintain
  • Easier to write tests for all the methods since the NestedBuilder (or whatever name fits) would just need to house the approx 20 where methods (might be less when after I'm done working on it).

I would love some feedback on this before I start coding 😉

@einar-hansen einar-hansen force-pushed the feature/whereAnyAllNone-dynamic branch from 74d94ce to 516bf8d Compare July 31, 2024 17:27
@einar-hansen
Copy link
Contributor Author

I'm creating a new PR this weekend to solve it like suggested in the last comment ;)

Thanks @chu121su12 and @johanrosenson

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.

4 participants