Ensure empty queries are returned as invalid (#33095) #33573
Conversation
Null queries were causing an ActionRequestValidationException. To work around this issue, null queries are reset to a "match all" query which was already the default value. Empty and whitespaces-only queries were already passing the test suite. Specific test cases have just been added to ensure that.
|
Pinging @elastic/es-core-infra |
There was a problem hiding this comment.
thanks a lot for your contribution @cbismuth ! I have reviewed it, left a couple of comments, and also realized that the original bug is not present anymore, so the PR is not really fixing it. That said I am not sure we should incorporate this fix given that it was submitted to fix an issue that is already fixed.
| String index = "some_index"; | ||
| createIndex(index, Settings.EMPTY); | ||
| QueryBuilder builder = QueryBuilders |
There was a problem hiding this comment.
I think that this test is not run anymore? It's fine to factor this code out but can you also leave the previous test?
There was a problem hiding this comment.
You're totally right @javanna, I've probably made a bad cut/paste to get a test snippet, I'll restore the previous test.
| @@ -1267,13 +1268,21 @@ public void testPutTemplateBadRequests() throws Exception { | |||
| assertThat(unknownSettingError.getDetailedMessage(), containsString("unknown setting [index.this-setting-does-not-exist]")); | |||
| } | |||
|
|
|||
| public void testValidateQuery() throws IOException{ | |||
| public void testValidateQueryWithNullQuery() throws IOException { | |||
There was a problem hiding this comment.
These tests are ok but they always use a query builder and I suspect they don't exactly reproduce setting the query to null or empty. I think that we should also add a test that leverages the logic we have on the REST layer, see RestValidateQueryAction#prepareRequest.
There was a problem hiding this comment.
Yes, I'll add a such a test.
There was a problem hiding this comment.
@javanna, after reviewing my own commit, I now realize why we didn't understand each other. I've read the issue description far too fast, I though it was written: "an empty query should be marked as valid" ... my bad, sorry for all that noise, these are my first baby steps as an Elasticsearch contributor.
javanna
added
:Search/Search
|
Pinging @elastic/es-search-aggs |
|
One more thing, I am not sure how this change relates specifically to the high-level REST client, can you clarify that? Maybe I am missing something. Thanks! |
|
You're welcome @javanna. I've added tests in the high-level REST client, that's why I've set this PR title. But your're right, the NPE check is in the server/action module. I'll develop a cURL recreation script and use a debugger to have a better understanding of how validation works. Then, I'll check if a REST integration test has been developed and add a new one if none exists. I'll let you know. |
cbismuth
changed the title
Null queries were causing an
ActionRequestValidationException. To work around this issue, null queries are reset to a "match all" query which was already the default value.Empty and whitespaces-only queries were already passing the test suite. Specific test cases have just been added to ensure that.