Support queries where part of the query is using ExactSettings

[damien-git]

Exact queries (surrounded with quotes) are very useful to eliminate issues with stemming. But they don't work when combined with non-exact queries. Some examples:

• translate AND "illustrate" : matches records with illustrated
• "robot" AND illustrate : matches records with robotics
• "covidence" AND review* : matches records with COVID

This PR proposes a solution for these cases. The implementation idea is to transform these queries into advanced queries, which use embedded Solr queries. Currently a workaround is to build equivalent queries as advanced queries in the UI.

It does not support multiple exact strings, or queries using field names (with ':') or with parenthesis. Logical operators are supported, including NOT for the exact part. The implementation could be improved in the future, but reported cases from our users did not include more complex queries.

[demiankatz]

Thanks, @damien-git. I confess that I haven't devoted particularly deep thought to the actual processing logic (though I at least read through it)... but here's a review consisting of some nitpicky suggestions as well as some higher-level thinking about the solution.

[demiankatz]

Would be nice to use QueryInterface for greater flexibility, and have more explicit types. Maybe something like:

Suggested change

	* @param QueryGroup|Query $query User query
	*
	* @return QueryGroup|Query
	*/
	protected function possiblyConvertMixedExactQueryIntoAdvanced($query)
	{
	if ($query instanceof QueryGroup) {
	return $query;
	}
	* @param QueryInterface $query User query
	*
	* @return QueryInterface
	*/
	protected function possiblyConvertMixedExactQueryIntoAdvanced(QueryInterface $query): QueryInterface
	{
	if (!($query instanceof Query)) {
	return $query;
	}

[damien-git]

Initially I was using AbstractQuery, which is used in the calling code and implements QueryInterface. But my editor (with the intelephense extension) was complaining that it doesn't define getHandler() (which is only in Query). I tried for a while to do a cast and use a different variable with the Query type specified in an inline comment. It worked but it was ugly. Then I thought of using QueryGroup|Query. The type checker is smart enough to realize that if the initial type is QueryGroup|Query and if QueryGroup objects are discarded, it only leaves the Query type, and it no longer complains about the method call. I like this solution better, because it clarifies the expected types in the function definition.
Actually, now that I think about it, I have a better option: use QueryInterface or AbstractQuery, and return the query if it's not an instance of Query. I am not sure what WorkKeysQuery is for, but it would better handle that case. Note that there are similar issues with query types elsewhere in QueryBuilder.

[demiankatz]

Agreed, that's why I suggested the !($query instanceof Query) check at the top of the method.

It may be worthwhile to invest some effort into cleaning up the other type issues you mention as a dev-12.0 PR (I'm sure problems have accumulated over time as the code has evolved), but I'll leave it to you whether or not now is the time for that. :-)

[damien-git]

I will go with your solution, just using AbstractQuery instead of QueryInterface to be more consistent with the rest of the code in the file.

[demiankatz]

Since AbstractQuery implements QueryInterface, I think using QueryInterface would be preferable as it is a more general option. (In practice, I think it likely makes no real difference one way or the other -- but using the interface feels better from a design perspective).

[demiankatz]

That being said, if you think it's better to use AbstractQuery for now, and then follow up with a dev-12.0 PR that switches all AbstractQuery references to QueryInterface for consistency, I could live with that approach. :-)

[demiankatz]

Would this part be better handled as a separate test using a data provider?

[damien-git]

It is consistent with testNormalization(). Are you thinking of something like getQuestionTests() ?

[demiankatz]

Ahh, these existing tests are very old and not using best practices. Even getQuestionTests should be refactored to use a proper dataProvider. Would it be helpful for me to open a PR to modernize the existing tests so you have a model to work from here? I can probably find time for that tomorrow if it would be useful.

[demiankatz]

I opened #4981 as a demonstration of the sort of change I had in mind.