"query_string" parser has changed in 7.x to allow bad percolator queries

[ymsoftware]

Elasticsearch version (bin/elasticsearch --version): 7.9.1

Plugins installed: []

JVM version (java -version):

OS version (uname -a if on a Unix-like system): Amazon hosted, Windows

Description of the problem including expected versus actual behavior:
"query_string" parser has changed in 7.x to allow some bad percolator queries; its validation is less strict than it used to be in previous versions (6.x)

Steps to reproduce:

Please include a minimal but complete recreation of the problem,
including (e.g.) index creation, mappings, settings, query etc. The easier
you make for us to reproduce it, the more likely that somebody will take the
time to look at it.

1. curl -X PUT "localhost:9200/test" -H 'Content-Type: application/json' -d' { "settings": { "index": { "number_of_shards": "1", "number_of_replicas": "0" } }, "mappings": { "dynamic": "false", "properties": { "query": { "type": "percolator" }, "message": { "type": "text" } } } } '
2. curl -X PUT "localhost:9200/test/_doc/1" -H 'Content-Type: application/json' -d' { "query": { "query_string": { "query": "message:(xyz:abc)" } } } '
   Error is expected: { "type": "query_shard_exception", "reason": "No field mapping can be found for the field with name [xyz]", "index": "test" }
3. curl -X PUT "localhost:9200/test/_doc/1" -H 'Content-Type: application/json' -d' { "query": { "query_string": { "query": "message:(xyz:\"abc\")" } } } '
   Success is not expected: { "_index": "test", "_type": "_doc", "_id": "1", "_version": 1, "result": "created", "_shards": { "total": 1, "successful": 1, "failed": 0 }, "_seq_no": 0, "_primary_term": 1 }

Provide logs (if relevant):

[elasticmachine]

Pinging @elastic/es-search (Team:Search)

[jtibshirani]

Hello @ymsoftware, I looked into this and it indeed seems like there was a change to percolator query parsing in 7.7. However I'm not sure the old behavior was preferable. I tried running this query, and it succeeds on 6.8 and the 7.x series:

POST test/_search
{
  "query": {
    "query_string": {
        "query": """message:(xyz:"abc")"""
      }
    }
}

So the current percolator parsing logic is more consistent with how we usually parse these queries.

Is there some way in which causes problems for your application, or were you just surprised to see a difference between versions? If it's the latter, maybe we can just close this out.

[ymsoftware]

Hi,
It is not about percolation/search, it is about indexing bad queries that will never be matched in percolation/search. Elastic percolator tries to validate queries before indexing them; percolation is an expensive process, the less junk percolator has to go through the faster it works. The pre 7.9 (I tested it on 6.x) percolator was slightly more restrictive and would throw an error on indexing bad queries more often than 7.9 does.
In my description both queries use the same field "xyz" and same value "abc", with and without quotes. The field is not part of the index's schema and both queries should fail on indexing. Both queries do fail in 6.x version, but in 7.9 the second one gets indexed. (To be fair, this is an extremely bad query, I don't really understand how it would work even if the "xyz" field was part of the schema, but it is another story)
Yes, that quality of queries should be enforced upstream by consumers. We are doing everything to keep bad queries away from our percolator and our application will continue to work whether or not you fix this.

[jimczi]

Agreed, I think we changed how unmapped fields are checked when expanding the target field list in our query parsers. That removed the validation which in the context of the percolator throw an error in presence of an unknown field. We should fix that change to call SearchExecutionContext#getFieldType consistently.

[jtibshirani]

Thanks @ymsoftware and @jimczi for the additional details. I see the problem now, before I didn't understand that percolator parsing is stricter than usual query parsing in terms of how it handles unmapped fields.

From testing, this change actually happened in 7.7 so I'm not sure #63322 is the reason. As a guess #52486 might be relevant, which started to rewrite percolator queries prior to storing them, and had a small refactor. I agree we should look into why this changed.

[kuan-clarabridge]

Have you found the reason why the behavior changed? Will this new behavior an official feature or is subject to change? For our case, I don't know whether we can rely on this behavior to allow the unmapped field in the percolator query. Previously we had code to validate the percolator query.

[elasticsearchmachine]

Pinging @elastic/es-search-relevance (Team:Search Relevance)