Views search filter needs additional WHERE on search_index table

As a note, when you create this content and use the main Search box to search, you get the right results. If you search for "one", the Testing one one one node comes up first. So this problem seems to be specific to Views. I'll take a look...

Interestingly, I am only seeing this problem when I make Page nodes with empty bodies. I first tested by creating Article nodes with a body of "This is a test" and I did not see this problem. But once I created Page nodes with empty body, I could see this problem. But still both my articles and my pages are working correctly outside Views in a normal content search.

So let's see what the difference is in the queries. When I search for "one" in the views preview area, Views is telling me the query is:

SELECT node_field_data.changed AS node_field_data_changed,
SUM(node_search_index.score * search_total.count) AS score,
MIN(node_field_data.nid) AS nid,
MIN(users_field_data_node_field_data.uid) AS users_field_data_node_field_data_uid
FROM
{node_field_data} node_field_data
INNER JOIN {users_field_data} users_field_data_node_field_data ON node_field_data.uid = users_field_data_node_field_data.uid
LEFT JOIN {search_index} node_search_index ON node_field_data.nid = node_search_index.sid AND (node_search_index.type = 'node_search' AND node_search_index.langcode = node_field_data.langcode)
LEFT JOIN {search_total} search_total ON node_search_index.word = search_total.word
INNER JOIN {search_dataset} node_search_dataset ON node_search_index.sid =
node_search_dataset.sid AND (node_search_index.type = node_search_dataset.type
AND node_search_index.langcode = node_search_dataset.langcode)

WHERE (( (node_field_data.status = 1 OR (node_field_data.uid =
***CURRENT_USER*** AND ***CURRENT_USER*** <> 0 AND
***VIEW_OWN_UNPUBLISHED_NODES*** = 1) OR ***BYPASS_NODE_ACCESS*** = 1) AND(
(node_search_index.type = 'node_search') AND (node_search_dataset.data LIKE '%
one %' ESCAPE '\\') )))

GROUP BY node_search_index.sid, node_field_data_changed
HAVING (( (COUNT(*) >= '1') ))
ORDER BY score DESC

I added a drupal_set_message to the SearchQuery class to show me the query on the Search page, and that is showing:

SELECT i.langcode AS langcode,
 i.type AS type,
 i.sid AS sid,
 SUM(((ROUND(:normalization_0, 4)) * i.score * t.count)) AS calculated_score
 FROM {search_index} i 
INNER JOIN {node_field_data} n ON n.nid = i.sid
 INNER JOIN {search_total} t ON i.word = t.word
 INNER JOIN {search_dataset} d ON i.sid = d.sid AND i.type = d.type AND
 i.langcode = d.langcode

 WHERE (n.status = :db_condition_placeholder_0) AND( (i.word =
 :db_condition_placeholder_1) )AND (i.type = :db_condition_placeholder_2) AND(
 (d.data LIKE :db_condition_placeholder_3 ESCAPE '\\') )

 GROUP BY i.langcode, i.type, i.sid
 HAVING (COUNT(*) >= :matches)
 ORDER BY calculated_score DESC LIMIT 10 OFFSET 0 

Very odd. I do not see a huge difference in those queries. It looks like the ORDER BY is the same, plus or minus using different table aliases in the two queries.

Oh wait. The views query has

WHERE node_search_dataset.data LIKE '% one %' ESCAPE '\\'

And the SearchQuery query has

WHERE i.word = :db_condition_placeholder_1) )
AND ((d.data LIKE :db_condition_placeholder_3 ESCAPE '\\') )

So that is the difference. The views query is not filtering the search_index table down to the right word, when it is adding up the scores. That is a bug.

Explanation: The search_index table keeps track of all the words found in the node, and assigns each one a score for that node. If you don't filter the scores based on the words in the actual query, you're basically asking "What is the total score of this node" not "What is the total score for the words I am searching on in this node".

So the relevance score is not working right for Views. We need to fix this in the Search filter and Search argument plugins for Views. And add more tests for them most likely.

The problem here is that the Views plugins are calling SearchQuery::parseSearchExpression() and assuming that the search query is fully set up with conditions, that the Views plugins then add to the query.

But the i.word conditions are not added at that point, so they are not added to the Views queries. Those are added in the prepareAndNormalize() method, which the Views queries don't want to deal with and shouldn't, because that assumes different search rankings as set up on the search plugin, and Views only offers relevance. But it needs the i.word expressions.

Here's a patch that fixes the problem. Probably needs a test.

As a note there is a small docs fix that is technically unrelated to the issue here in the patch. We can move it to another issue, but I ran into the docs being wrong when exploring solutions to the issue so it got left in.

Oops I forgot the patch.

Spun the docs part from ViewsSearchQuery into its own issue.
#2544870: Docs problem on ViewsSearchQuery::conditionReplaceString() so that part should be removed from this patch. Since this patch still needs tests I'll wait.

Summary update

Regarding tests, we have SearchRankingTest in the Search module, which tests ranking for the main search functionality. This is more complex than the behavior in Views, because the main search page offers configurable ranking (you can set all kinds of factors, not just search keyword relevance).

That test makes a node whose body is "Drupal's search rocks", and then another one that says "Drupal's search rocks really rocks", and it verifies that the order is right if you search for "rocks" (i.e., that it's "rocks rocks" before "rocks").

The other test we have is in the Views module, SearchIntegrationTest, which tests the search filter and argument plugins.

So... I think the thing to do would be to add to SearchIntegrationTest. We should make nodes similar to the ones used to test relevant rankings in SearchRankingTest, and verify that they appear in the right order when searching via the test-filter and test-arg views that are set up in SearchIntegrationTest.

If you want to right a test, @jhedstrom, please do! That would be my suggestion of how to do it. Of course, attaching a test-only patch to verify that it fails would be ideal. :)

Come to think of it, we should consider adding to both SearchRankingTest and SearchIntegrationTest the exact test strings you came up with in this test, because some variations do not exhibit the problem.

Interesting. It looks like the "one two two"/"one one one" test is the better one for this bug.

+++ b/core/modules/search/src/ViewsSearchQuery.php
@@ -69,8 +69,9 @@ public function publicParseSearchExpression() {
-   * @param \Drupal\Core\Database\Query\Condition $condition
-   *   The query condition in which the string is replaced.
+   * @param array $condition
+   *   The query conditions array in which the string is replaced. This is an
+   *   item from a $condition->conditions array, which has a 'field' element.
    */

We need to take this bit out of the patch (I moved it to a different issue).

Other than that, looks great! So I'll just edit the patch and take that file's section out. (Oh, the horror!). Attached patch is the same as #14 but without ViewsSearchQuery.

I hereby pronounce the test part of the patch RTBC. I cannot RTBC the code part of the patch as I wrote it.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 7e4c9d7 and pushed to 8.0.x. Thanks!

Tested on postgres as well cause views sorting can be different - test passes.