Do not add empty query

[hulkur]

If there are no where clauses then SearchFactory adds query string filter even if query string is empty

[matchish]

LGTM. Could you update the Changelog.md?

[matchish]

@hulkur @hkulekci This PR changes observable behavior which typically requires a major version bump. However, since I can't identify any realistic scenarios where this would break existing implementations, a PATCH bump should suffice. Do you see any potential breaking changes that would warrant a major version bump?

[hkulekci]

@hulkur @matchish To understand the problem lets prepare a sample. Even we can put these samples into test cases. It would be better. This is the current behaviour:

    public function test_where_clauses_are_added_to_query_2(): void
    {
        $builder = new Builder(new Product(), '*');
        $search = SearchFactory::create($builder);
        $this->assertInstanceOf(BoolQuery::class, $search->getQueries());

        $this->assertEquals(
            [
                'query_string' => [
                    'query' => '*',
                ],
            ],
            $search->getQueries()->toArray()
        );
    }

Our expectation is to remove the query string from here. I just apply your changes and write this test:

    public function test_where_clauses_are_added_to_query_3(): void
    {
        $builder = new Builder(new Product(), '');
        $search = SearchFactory::create($builder);
        $this->assertNull($search->getQueries());

        $this->assertEmpty($search->toArray());
    }

To achieve this, we need to send an empty string to the builder. However, this behavior hides this feature. Instead of sending an empty string, we need to make it nullable. So, this change could be a BC break or not. It wont break anything but also change the behaviour.

Lastly, we can even remove the query string if we have a where query and with empty string. The test will change from

    public function test_where_clauses_are_added_to_query(): void
    {
        $builder = new Builder(new Product(), '*');
        $builder->where('price', 100);

        $search = SearchFactory::create($builder);

        $this->assertNotEmpty($search);
        $this->assertInstanceOf(BoolQuery::class, $search->getQueries());

        $this->assertEquals(
            [
                'bool' => [
                    'filter' => [
                        [
                            'term' => ['price' => 100]
                        ]
                    ],
                    'must' => [
                        [
                            'query_string' => [
                                'query' => '*',
                            ],
                        ],
                    ]
                ],
            ],
            $search->getQueries()->toArray()
        );
    }

to

    public function test_where_clauses_are_added_to_query_4(): void
    {
        $builder = new Builder(new Product(), '');
        $builder->where('price', 100);

        $search = SearchFactory::create($builder);

        $this->assertNotEmpty($search);
        $this->assertInstanceOf(BoolQuery::class, $search->getQueries());

        $this->assertEquals(
            [
                'bool' => [
                    'filter' => [
                        [
                            'term' => ['price' => 100]
                        ]
                    ]
                ],
            ],
            $search->getQueries()->toArray()
        );
    }

As a result, it will improve the quality and change behavior. But we also need to inform users about these behavioral changes. I am not sure it requires a major bump. But could you please put these tests on your PR?

[hkulekci]

BTW, these are the results for the tests above with @hulkur changes:

 vendor/bin/phpunit --filter=test_where_clauses_are_added_to_query --testdox   
PHPUnit 9.6.22 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.4.1
Configuration: /Volumes/webroot/github/laravel-scout-elasticsearch/phpunit.xml.dist
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Search Factory (Tests\Unit\ElasticSearch\SearchFactory)
 ✔ Where clauses are added to query  348 ms
 ✔ Where clauses are added to query 2  20 ms
 ✔ Where clauses are added to query 3  23 ms
 ✔ Where clauses are added to query 4  21 ms

Time: 00:00.448, Memory: 32.00 MB

OK (4 tests, 10 assertions)

[hulkur]

In your test you create the Builder directly.
In my case I use Model::search() and then add callback to build up the final query.
Currently empty query is added before calling callback function

[matchish]

In my case I use Model::search() and then add callback to build up the final query.

Could you share how do you add callback after search as I remember scout allow to do it only in search(string, callback)

but you are right that's what I meant, do we have some case when we miss querystring and it could break some project code in the callback

$results = Product::search(NULL, function(\Elastic\Elasticsearch\Client $client, $body) {

    // someone expects that body contains QueryStringQuery and we break his code because after update no default QueryStringQuery there anymore
    
    return $client->search(['index' => 'products', 'body' => $body->toArray()])->asArray();
})->raw();

[hulkur]

Builder $callback is public variable, just assign a function.

[matchish]

Since the compatibility impact is still unclear, I've merged the changes to master but kept them unreleased. These updates will be included in the next major version release. In the meantime, you can access these changes directly from the master branch.