Trailing whitespace in the query is not supported

[bojanbogdanovic]

Solarium version(s) affected: 6.3.*
Solr version: all
Solr mode: standalone / cloud

Description
This is a follow-up of: https://www.drupal.org/project/search_api_solr/issues/3509593

There are Solr lookup implementations where you actually want a suggestion after a space, for example with the "FreeTextLookupFactory" and using the space as separator.

Solarium trims whitespace on the query, which limits the capabilities of Solr suggesters like "FreeTextLookupFactory".

How to reproduce
See: https://github.com/solariumphp/solarium/blob/6.3.7/src/Component/QueryTrait.php#L34

Possible Solution
Only trim on the left side of the query or remove trim function completely from the setQuery method.

[thomascorthals]

Only trim on the left side of the query or remove trim function completely from the setQuery method.

Are there any use cases where a left side space is relevant?

[bojanbogdanovic]

Not that I'm aware of. For me there is a specific case for the trailing space. I would remove the trim function, which is in my opinion cleaner and gives more flexibility.

[mkalkbrenner]

I think at least we should keep the trim for the left side.

Maybe it is right to leave all of that to the client and to not touch the query at all. On the other hand, it might help in some cases.
And it is in the lib since 15 years. Could be a breaking change.

What about introducing something like setRawQuery()?

[thomascorthals]

Right-to-left scripts might run into the same issue with only ltrim() as left-to-right scripts would with only rtrim(). I'd avoid trimming from one side only.

I'm in favour of the setRawQuery() idea because I can imagine changing this behaviour will break things for users. Or maybe make it configurable with the current trimming as the default?

[mkalkbrenner]

Right-to-left scripts might run into the same issue with only ltrim() as left-to-right scripts would with only rtrim(). I'd avoid trimming from one side only.

Good point!

I'm in favour of the setRawQuery() idea because I can imagine changing this behaviour will break things for users. Or maybe make it configurable with the current trimming as the default?

You're right. A configuration option would be better from client's perspective. Calling different methods requires code changes in any framework. Ideally the user could set the config option or implement an EventSubscriber without waiting for new release of a framework.

[thomascorthals]

The trim() is done in Solarium\Component\QueryTrait::setQuery(). Changes to that method directly affect:

• Component\DisMax\BoostQuery
• Component\Facet\JsonQuery
• Component\Facet\Query
• Component\Highlighting\Highlighting
• Component\ReRankQuery
• Component\Spellcheck
• Component\Suggester
• QueryType\Analysis\Query\Document
• QueryType\Analysis\Query\Field
• QueryType\Select\Query\FilterQuery
• QueryType\Select\Query\Query
• QueryType\Spellcheck\Query
• QueryType\Suggester\Query

That's not necessarily a problem. Implementing it as QueryTrait::setUseTrim() means you can set the config option differently for any query. Just something to keep in mind and to include in testing if implemented that way.

I wouldn't use setTrim() as method name to be on the safe side if a Solr component ever introduces a trim parameter.

[thomascorthals]

Maybe it is right to leave all of that to the client and to not touch the query at all. On the other hand, it might help in some cases.
And it is in the lib since 15 years. Could be a breaking change.

I agree that this should actually be left to the client. And now would be a good time to make such a possibly breaking change for Solarium 7.

Ideally the current behaviour would be deprecated first and removed in a later release. But there is no trivial way to do so in this case.

A convoluted way to do it over three major releases:

7. Make query trimming configurable.
   • Add a usetrim configuration option.
   • Still trim by default if usetrim isn't set.
   • Trigger a deprecation in setQuery() if usetrim isn't set, spurring users on to make an explicit choice, and informing them the default behaviour will change in the next major release.
   • Trigger a deprecation if usetrim is set to true, informing users that trimming will be removed in the next major release and should be handled in user code instead.
8. No longer trim any queries.
   • It's no longer mandatory to set usetrim.
   • Throw a RuntimeException if usetrim is set to true.
   • Trigger a deprecation if usetrim is set to false, informing users that the option will be removed in the next major release as it no longer has an effect on the query.
9. Remove the configuration option.
   • Ignore usetrim if it's still set.

That way nobody would be caught unaware unless they skip over a major version.

[thomascorthals]

Throw a RuntimeException if usetrim is set to true.

Or should this be E_USER_NOTICE/E_USER_WARNING?