Validate API with empty query returns HTTP code 400 instead of 200 and "valid": false

[nicky1038]

Elasticsearch 5.6.10.

Request

GET someindex/sometype/_validate/query?explain=true
{"query":{ }}

Results in:

{
  "error": {
    "root_cause": [
      {
        "type": "action_request_validation_exception",
        "reason": "Validation Failed: 1: query cannot be null;"
      }
    ],
    "type": "action_request_validation_exception",
    "reason": "Validation Failed: 1: query cannot be null;"
  },
  "status": 400
}

But it should return 200 and just say this query is not valid, e. g.

GET someindex/sometype/_validate/query?explain=true
{"query":{ blabla }}

Results in:

{
  "valid": false,
  "error": """
ParsingException[Failed to parse]; nested: JsonParseException[Unexpected character ('b' (code 98)): was expecting double-quote to start field name
 at [Source: org.elasticsearch.transport.netty4.ByteBufStreamInput@7081512b; line: 1, column: 13]];; com.fasterxml.jackson.core.JsonParseException: Unexpected character ('b' (code 98)): was expecting double-quote to start field name
 at [Source: org.elasticsearch.transport.netty4.ByteBufStreamInput@7081512b; line: 1, column: 13]
"""
}

Please fix it also in 5.6.x as we currently cannot easily upgrade to 6.x.

[dakrone]

I tried this out on master, it's still a bug but with a different error, instead, no response is sent (the connection is just closed) and we fail to send the error message:

[elasticsearch] [2018-08-23T09:04:39,317][ERROR][o.e.r.a.RestResponseListener] [node-0] failed to send failure response
[elasticsearch] java.lang.IllegalStateException: Channel is already closed
[elasticsearch] 	at org.elasticsearch.rest.RestController$ResourceHandlingHttpChannel.close(RestController.java:503) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.rest.RestController$ResourceHandlingHttpChannel.sendResponse(RestController.java:496) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.rest.action.RestActionListener.onFailure(RestActionListener.java:58) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.rest.action.RestActionListener.onResponse(RestActionListener.java:49) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.support.TransportAction$1.onResponse(TransportAction.java:66) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.support.TransportAction$1.onResponse(TransportAction.java:62) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.admin.indices.validate.query.TransportValidateQueryAction.lambda$doExecute$2(TransportValidateQueryAction.java:94) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.ActionListener$1.onFailure(ActionListener.java:68) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:62) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.index.query.Rewriteable.rewriteAndFetch(Rewriteable.java:114) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.index.query.Rewriteable.rewriteAndFetch(Rewriteable.java:87) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.admin.indices.validate.query.TransportValidateQueryAction.doExecute(TransportValidateQueryAction.java:108) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.admin.indices.validate.query.TransportValidateQueryAction.doExecute(TransportValidateQueryAction.java:62) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
[elasticsearch] 	at org.elasticsearch.action.support.TransportAction$RequestFilterChain.proceed(TransportAction.java:143) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]

[elasticmachine]

Pinging @elastic/es-search-aggs

[javanna]

Besides the missing response above which is pretty bad, I would say at first glance that it's ok for the validate query API to require a query to be provided. Looking at the code though, I see that the request has a match_all default, which sounds like maybe something has changed at some point in the REST layer where we end up setting null to the request while we should leave the default that is already set to the request? Maybe just speculating but these are my findings from looking at it for a couple of minutes.

[nicky1038]

@dakrone @javanna First of all, thank you for such a quick response.

@javanna Provided request body contains a field named "query", and its content is even a valid JSON (though it is empty), and my need is to find out if this JSON contains a valid query. Emptiness is not. I think this is enough and ES should respond with "is-valid": false but not with server error.

UPD: BTW even if there is no query in a request ES returns 200 as it is normal for validate API.

GET someindex/sometype/_validate/query?explain=true
{ 42 }

[cbismuth]

I've opened a pull request to fix this issue, see #33573.

[javanna]

I did some more digging and this has been fixed in 6.x. I can only reproduce the issue on 5.6 and that's due to this line which was later changed to better handle empty queries. In this specific case we now throw error rather than returning null, which is then caught and gets converted into an OK response with flag valid set to false.

I do not know if we plan to get another 5.6.x release out but I would not see this as a blocker that would be included in it anyways.

@dakrone I checked against master as well and I cannot repro what you mentioned (the missing response), could you check that again please? I would be leaning towards closing this issue but I would like to first clarify that bit too.

[cbismuth]

Thank you @javanna, I'll update my pull request with your comments.

[javanna]

hi @cbismuth as I said in the PR I am not sure that the PR is needed, given that the issue has already been fixed in 6.x and master. Do you see what I mean?

[cbismuth]

Yes @javanna, I see. I wanted to contribute a non-regression test if none was already present for the reported issue. Let me know if it's useful or not, no worry at all.

[javanna]

Contributing a unit test for this sounds good @cbismuth , thanks!

[cbismuth]

I've created a cURL recreation Bash script, see Gist here.

Here is the output on master with both empty query and malformed query:

[09-19-18 09:52:53] Delete [index1] index
{"acknowledged":true}

[09-19-18 09:52:53] Create [index1] index
{"acknowledged":true,"shards_acknowledged":true,"index":"index1"}

[09-19-18 09:52:53] Create document [1] in index [index1]
{"_index":"index1","_type":"type1","_id":"1","_version":1,"result":"created","forced_refresh":true,"_shards":{"total":2,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1}

[09-19-18 09:52:53] Validate empty query against index [index1] and type [type1]
{"valid":false,"error":"ParsingException[Failed to parse]; nested: IllegalArgumentException[query malformed, empty clause found at [3:5]];; java.lang.IllegalArgumentException: query malformed, empty clause found at [3:5]"}

[09-19-18 09:52:53] Validate invalid query (malformed JSON) against index [index1] and type [type1]
{"valid":false,"error":"ParsingException[Failed to parse]; nested: JsonParseException[Unexpected character ('4' (code 52)): was expecting double-quote to start field name\n at [Source: org.elasticsearch.transport.netty4.ByteBufStreamInput@7245c7b2; line: 3, column: 10]];; com.fasterxml.jackson.core.JsonParseException: Unexpected character ('4' (code 52)): was expecting double-quote to start field name\n at [Source: org.elasticsearch.transport.netty4.ByteBufStreamInput@7245c7b2; line: 3, column: 10]"}

As @javanna said, issue has been fixed.

I'll dig into the source code to check if REST test cases have been developed to validate empty and malformed queries.

[cbismuth]

Contributing a unit test for this sounds good @cbismuth , thanks!

Unit test contribution is done in PR #33862.