Empty @search prevents ScoutBuilderDirective from executing

[Nextra]

Describe the bug

Empty values for @search arguments are a valid search use case. Search engines like Meilisearch will return a (more or less useful) set of results even for empty queries. Currently in lighthouse however, the ScoutEnhancer changes its behavior when the search string is null or ''. This is due to these code pieces interacting:

lighthouse/src/Scout/ScoutEnhancer.php

Lines 98 to 100 in 889c7bc

	if ($argumentHasSearchDirective && is_string($argument->value)) {
	$this->searchArguments[] = $argument;
	}

lighthouse/src/Scout/ScoutEnhancer.php

Lines 51 to 54 in 889c7bc

	public function hasSearchArguments(): bool
	{
	return $this->searchArguments !== [];
	}

lighthouse/src/Scout/ScoutEnhancer.php

Lines 62 to 65 in 889c7bc

	public function wouldEnhanceBuilder(): bool
	{
	return $this->hasSearchArguments();
	}

First, the search argument is not added to the list of search arguments if the value is empty. Then, when the enhancer is probed, it claims there is nothing to be done with the ScoutBuilder.

I don't quite understand the rest of the control flow enough, but the search itself will be executed correctly. The field will be resolved using a ScoutBuilder, and the empty query will be handled by the responsible search engine as expected. However, when other ScoutBuilderDirective are placed on the argument, they will not be allowed to handle the builder while the search query value is empty. I think ScoutBuilderDirective should be able to add additional filters regardless of the search value.

Expected behavior/Solution

Any ScoutBuilderDirective that wishes to enhance the scout builder should be executed even for empty @search values.

Removing the is_string check shown above seems to fix the behavior at first glance, but I don't know if this has deeper repercussions.

Steps to reproduce

1. Define a query field with an argument that uses @search to enable scout
2. Build any ScoutBuilderDirective that, for example, places a simple $builder->where('foo', 'bar') on the builder
3. Observe the builder directive not executing for null or '' search strings.

Lighthouse Version

v6.22.0

[spawnia]

First, the search argument is not added to the list of search arguments if the value is empty.

It is not added when the value is null, which in GraphQL is usually considered to be the same as the argument not being present at all. Perhaps you are using Nuwave\Lighthouse\Schema\Directives\ConvertEmptyStringsToNullDirective, which is included in the default field_middleware?

However, when other ScoutBuilderDirective are placed on the argument, they will not be allowed to handle the builder while the search query value is empty

My understanding was that going from Illuminate\Database\Eloquent\Builder to Laravel\Scout\Builder requires calling Laravel\Scout\Searchable::search() with a non-null query string. Can you add a test with an example where no argument with @search is used and it still correctly applies any Nuwave\Lighthouse\Scout\ScoutBuilderDirective?

[Nextra]

It is not added when the value is null, which in GraphQL is usually considered to be the same as the argument not being present at all. Perhaps you are using Nuwave\Lighthouse\Schema\Directives\ConvertEmptyStringsToNullDirective, which is included in the default field_middleware?

Yes, I am using that Middleware.

My understanding was that going from Illuminate\Database\Eloquent\Builder to Laravel\Scout\Builder requires calling Laravel\Scout\Searchable::search() with a non-null query string.

Seems to be fine with accepting null, and it behaves equally to an empty string, at least for Meilisearch. Tinker:

> User::search(null)
= Laravel\Scout\Builder {#8066}
> User::search(null)->get()->count()
= 20
> User::search(null)->get() == User::search('')->get()
= true

Can you add a test with an example where no argument with @search is used and it still correctly applies any Nuwave\Lighthouse\Scout\ScoutBuilderDirective?

You are correct. Leaving the empty value means that the field query still works, but it is no longer using scout. It seems like I didn't check properly. I think it is somewhat counter-intuitive because I definitely need fields that will still search on empty values, but if this is expected behavior I will try to make my own way around this.

[spawnia]

The issue I see is that queries may be written in a way to use Scout or database queries.
Keep in mind that @eq can apply to both.

type Query {
  users(name: String @search, email: String @eq): [User!]!
}

Given the following query string, let's explore what the different variable configurations would do:

query ($name: String, $email: String) {
  users(name: $name, email: $email) { ... }
}

Anything with name forces a Scout search:

{"name": "foo"}
{"name": "foo", "email": "[email protected]"}

The following do not use name, so I would expect them to use a database query.
Given the use of variables, we can not really differentiate between them:

{"email": "[email protected]"}
{"email": "[email protected]", "name": null}

[spawnia]

Let's also consider this from a schema evolution perspective. We start with the following schema, not using Scout.

type Query {
  users(email: String @eq): [User!]!
}

Clients query it like this:

{ users(email: "[email protected]") { id } }

Now, we add an additional argument with @search.

type Query {
  users(name: String @search, email: String @eq): [User!]!
}

Suppose that we always use Scout if there is a search argument, even if it is null.
The behaviour for the client that still sends the query now changes, their query now uses Scout.

[Nextra]

Interesting, I understand what is happening now. I never considered it that way, and added unique fields for the search variants - userSearch for this example. I guess this is why I was looking for the wrong thing (or in the wrong place).

I will see how to achieve what I expected to happen. I guess it is only the interaction of the empty strings to null middleware that ultimately "breaks" what I want to do, so it shouldn't be a massive issue.

Thanks.

[spawnia]

unique fields for the search variants

Probably the better way to go. The current approach is somewhat mysterious to clients and can result in runtime exceptions, e.g.

lighthouse/src/Scout/ScoutEnhancer.php

Line 138 in 6b0fdde

throw new ScoutException('Found arg builder arguments that do not work with @search.');

Laravel's Illuminate\Foundation\Http\Middleware\ConvertEmptyStringsToNull provides a method skipWhen that allows conditional exclusion. Perhaps we could add a directive such as @preserveEmptyStrings to allow skipping @convertEmptyStringsToNull for certain arguments, while still keeping it generally?

[Nextra]

I think you technically already have that mechanism, it's String!.

Part of my misunderstanding was that passing null would lead to an empty search, so I used that out of convenience. What I should actually do is use String! and make sure the clients explicitly request that empty query.

[spawnia]

Perhaps we can describe that behaviour in the @search docs?

[Nextra]

I don't know how common this misunderstanding is, maybe I'm just thinking about this whole GraphQL thing wrong :)

Those runtime interactions you detailed above seem to be worth noting as caveats. They feel quite unintuitive to me, and the behavior with empty/null string arguments would just naturally come up.

[Nextra]

While playing around with the String! scenario I found something else. For this schema:

type Query {
    userSearch(search: String! @search) [User!]!
}

The following query will work even with the middleware active, and behave differently depending on whether the argument is declared as String or String!. I assume this is because the middleware recognizes it is not allowed to pass null into the resolver.

query {
    userSearch(search: "") {
        id
    }
}

However, doing the same with query:

query UserSearch($search: String!) {
    userSearch(search: $search) {
        id
    }
}

and variables

{
  "search": ""
}

will produce the error Variable "$search" of non-null type "String!" must not be null..