Raise IllegalArgumentException instead if query validation failed #26811

liketic · 2017-09-28T11:37:22Z

Resolves #26799 . I'm trying to raise a IllegalArgumentException if the validation is failed. But I think it maybe not the best resolution, happy to hear your comments.

elasticmachine · 2017-09-28T11:37:24Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

bleskes · 2017-09-28T12:25:52Z

@javanna it seems you were involved in the discuss on the relevant ticket. Care to take a look?

javanna

Thanks a lot for your PR! I would have gone for IllegalArgumentException too, good choice. Adding a new exception has the disadvantage of needing to be supported by our serialization layer, which is a bit overkill in this case. Would you be up for adding a bit more test coverage? Some of the messages that you changed are not tested at all, it would be nice for instance to have a unit test for DefaultSearchContext and test the preProcess method if possible?

javanna · 2017-10-05T10:11:04Z

core/src/main/java/org/elasticsearch/search/DefaultSearchContext.java

I would also make the same change for "Cannot use [sort] option in conjunction with [rescore]." .

javanna · 2017-10-05T11:50:51Z

rest-api-spec/src/main/resources/rest-api-spec/test/scroll/12_slices.yml

we are not testing here the response code. Better to replace with catch: bad_request. That way we don't check the message, but we make sure that 400 is returned.

liketic · 2017-10-05T12:12:32Z

OK. Will do. Thanks for reviewing this PR.

liketic · 2017-10-06T10:47:41Z

@javanna Please review again.

javanna · 2017-10-06T16:33:04Z

core/src/test/java/org/elasticsearch/search/DefaultSearchContextTests.java

this one will start a node, which makes it more of an integration test, would it be possible to make it extend ESTestCase?

Updated. Please review again.

liketic · 2017-10-09T06:33:15Z

@javanna Could you please review again? Thanks in advance.

javanna · 2017-10-09T11:01:59Z

@elasticmachine test this please

javanna · 2017-10-09T12:01:24Z

retest this please

liketic · 2017-10-09T12:55:32Z

@javanna Seems like the testing is failed. Can I rebase this branch?

javanna · 2017-10-10T09:34:21Z

@liketic feel free to rebase

liketic · 2017-10-10T10:17:35Z

@javanna Rebased. Please run test again. Thanks.

javanna · 2017-10-10T10:20:25Z

retest this please

liketic · 2017-10-10T13:25:36Z

@javanna I think the test failure is unrelated. Any thoughts?

javanna · 2017-10-10T13:38:23Z

@liketic the last failure is related to your changes. What fails is a backwards compatibility test, which makes sense as your change is breaking backwards compatibility, although for a good cause.

Failure at [scroll/12_slices:106]: expected [400] status code but api [search] returned [500 Internal Server Error] [{"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"The number of slices [1025] is too large. It must be less than [1024]. This limit can be set by changing the [index.max_slices_per_scroll] index level setting.","stack_trace":"[The number of slices [1025] is too large. It must be less than [1024]. This limit can be set by changing the [index.max_slices_per_scroll] index level setting.]; nested: IllegalArgumentException[The number of slices [1025] is too large. It must be less than [1024]. This limit can be set by changing the [index.max_slices_per_scroll] index level setting.];

When running scroll/12_slices/Sliced scroll with invalid arguments against previous versions of Elasticsearch, a different response code is returned compared to what the updated test expects now. You need to add a skip section based on version as follows:

  - skip:
      version: " - 6.0.0-rc1"
      reason: Prior versions return 500 rather than 404

liketic · 2017-10-11T10:59:32Z

@javanna Thanks a lot! I tried to add this section to yaml, but I found the yaml parser cannot parse version 6.0.0-rc1, I have to create another PR #26964 to fix that. May I ask you to review that too?

javanna · 2017-10-11T11:52:40Z

@liketic my bad, we should rather skip " - 6.99.99" as this change will go to master only (and will be released with 7.0). Would you also mind adding a note to the migrate guide (migrate_7_0.asciidoc) about it?

… 6.99.99

javanna · 2017-10-11T12:05:52Z

retest this please

liketic · 2017-10-14T13:32:30Z

@javanna Thanks for your patient. I updated the doc, any comments are appreciated.

javanna · 2017-10-23T08:32:41Z

retest this please

javanna

hi @liketic thanks for your changes, I left a couple more comments but this is very close!

javanna · 2017-10-23T14:09:51Z

docs/reference/migration/migrate_7_0/search.asciidoc

 --------------------------------------------------
 // CONSOLE

+=== Scroll search will response `bad request` if request is invalid


I think we can skip the java exception names here given that this guide is read by all our users, also the ones that are not familiar with Java. Let's focus on response codes only. Also I would make the title shorter:

=== `_search/scroll` returns `400` for invalid requests The `/_search/scroll` endpoint returns `400 - Bad request` when the request invalid, while it would previously return `500 - Internal Server Error` in such case.

javanna · 2017-10-23T14:16:37Z

...rg/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregationBuilder.java

        if (filters.size() > maxFilters){
-            throw new QueryPhaseExecutionException(context,
+            throw new IllegalArgumentException(
                    "Number of filters is too large, must be less than or equal to: [" + maxFilters + "] but was ["


could we have a unit test for this change too?

javanna · 2017-10-23T14:17:19Z

core/src/test/java/org/elasticsearch/search/DefaultSearchContextTests.java

+
+public class DefaultSearchContextTests extends ESTestCase {
+
+    public void testPreProcess() throws Exception {


thanks a lot for taking the time to write this!

javanna · 2017-10-23T14:18:20Z

core/src/main/java/org/elasticsearch/search/SearchService.java

    private void contextScrollKeepAlive(SearchContext context, long keepAlive) throws IOException {
        if (keepAlive > maxKeepAlive) {
-            throw new QueryPhaseExecutionException(context,
+            throw new IllegalArgumentException(


do you think it would be possible to have a unit test for this too?

It's not easy to add unit test for this. The SearchServiceTests is also extend ESSingleNodeTestCase for other test cases. And this has been covered by the integration test. I can try to add a unit test if you think it's necessary.

javanna · 2017-10-24T14:44:49Z

...asticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregationBuilderTests.java

+        }
+        AdjacencyMatrixAggregationBuilder builder = new AdjacencyMatrixAggregationBuilder("dummy", filters);
+        try {
+            builder.doBuild(context, null, new AggregatorFactories.Builder());


can you use expectThrows here instead?

my bad. Thanks.

javanna · 2017-10-24T14:46:10Z

...asticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregationBuilderTests.java

+        AggregatorFactory<?> factory = builder.doBuild(context, null, new AggregatorFactories.Builder());
+        assertThat(factory instanceof AdjacencyMatrixAggregatorFactory, is(true));
+        assertThat(factory.name(), equalTo("dummy"));
+    }


thanks a lot for adding this test!

javanna

I did a last review round, this looks good, I left one minor remark but nothing blocking, I will merge once you made that minor change.

liketic · 2017-10-27T15:47:46Z

Hi @javanna , can we merge this now? Thanks! 😃

javanna · 2017-10-31T11:17:50Z

thanks @liketic merged to the master branch.

Relates to #26811

* master: Enhances exists queries to reduce need for `_field_names` (elastic#26930) Added new terms_set query Set request body to required to reflect the code base (elastic#27188) Update Docker docs for 6.0.0-rc2 (elastic#27166) Add version 6.0.0 Docs: restore now fails if it encounters incompatible settings (elastic#26933) Convert index blocks to cluster block exceptions (elastic#27050) [DOCS] Link remote info API in Cross Cluster Search docs page Fix Laplace scorer to multiply by alpha (and not add) (elastic#27125) [DOCS] Clarify migrate guide and search request validation Raise IllegalArgumentException if query validation failed (elastic#26811) prevent duplicate fields when mixing parent and root nested includes (elastic#27072) TopHitsAggregator must propagate calls to `setScorer`. (elastic#27138)

* master: Remove checkpoint tracker bit sets setting Fix stable BWC branch detection logic Fix logic detecting unreleased versions Enhances exists queries to reduce need for `_field_names` (elastic#26930) Added new terms_set query Set request body to required to reflect the code base (elastic#27188) Update Docker docs for 6.0.0-rc2 (elastic#27166) Add version 6.0.0 Docs: restore now fails if it encounters incompatible settings (elastic#26933) Convert index blocks to cluster block exceptions (elastic#27050) [DOCS] Link remote info API in Cross Cluster Search docs page Fix Laplace scorer to multiply by alpha (and not add) (elastic#27125) [DOCS] Clarify migrate guide and search request validation Raise IllegalArgumentException if query validation failed (elastic#26811) prevent duplicate fields when mixing parent and root nested includes (elastic#27072) TopHitsAggregator must propagate calls to `setScorer`. (elastic#27138)

* master: Lazy initialize checkpoint tracker bit sets Remove checkpoint tracker bit sets setting Fix stable BWC branch detection logic Fix logic detecting unreleased versions Enhances exists queries to reduce need for `_field_names` (#26930) Added new terms_set query Set request body to required to reflect the code base (#27188) Update Docker docs for 6.0.0-rc2 (#27166) Add version 6.0.0 Docs: restore now fails if it encounters incompatible settings (#26933) Convert index blocks to cluster block exceptions (#27050) [DOCS] Link remote info API in Cross Cluster Search docs page Fix Laplace scorer to multiply by alpha (and not add) (#27125) [DOCS] Clarify migrate guide and search request validation Raise IllegalArgumentException if query validation failed (#26811) prevent duplicate fields when mixing parent and root nested includes (#27072) TopHitsAggregator must propagate calls to `setScorer`. (#27138)

rjernst · 2018-03-20T15:23:43Z

I'm going to backport this to 6.x. I do not view this as breaking, the previous behavior was not intended, and it would only affect a user who is catching this error and trying to decide what to do based on it, and action based on that catching would likely be incorrect.

javanna self-requested a review September 28, 2017 13:16

javanna self-assigned this Sep 28, 2017

javanna requested changes Oct 5, 2017

View reviewed changes

javanna added :Search/Search Search-related issues that do not fall into other categories >enhancement labels Oct 5, 2017

javanna reviewed Oct 6, 2017

View reviewed changes

Raise IllegalArgumentException instead if query validation failed

f421d9a

liketic force-pushed the bugfix/issues/26799 branch from 4ce89b4 to f421d9a Compare October 10, 2017 10:16

Skip testing sliced scroll with invalid arguments for versions before…

d563e19

… 6.99.99

Update migration document

0280eba

javanna added v7.0.0 >breaking labels Oct 23, 2017

javanna removed the >enhancement label Oct 23, 2017

javanna requested changes Oct 23, 2017

View reviewed changes

Add unit test

f41e781

javanna reviewed Oct 24, 2017

View reviewed changes

javanna approved these changes Oct 24, 2017

View reviewed changes

Simplify exception assertion

08bf2e5

javanna added the >bug label Oct 30, 2017

javanna merged commit c3e2bdf into elastic:master Oct 31, 2017

liketic deleted the bugfix/issues/26799 branch October 31, 2017 11:18

javanna added a commit that referenced this pull request Oct 31, 2017

[DOCS] Clarify migrate guide and search request validation

3466684

Relates to #26811

rjernst removed the >breaking label Mar 20, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		public class DefaultSearchContextTests extends ESTestCase {

		public void testPreProcess() throws Exception {

Raise IllegalArgumentException instead if query validation failed #26811

Raise IllegalArgumentException instead if query validation failed #26811

Uh oh!

Conversation

liketic commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 28, 2017

Uh oh!

bleskes commented Sep 28, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liketic commented Oct 5, 2017

Uh oh!

liketic commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liketic commented Oct 9, 2017

Uh oh!

javanna commented Oct 9, 2017

Uh oh!

javanna commented Oct 9, 2017

Uh oh!

liketic commented Oct 9, 2017

Uh oh!

javanna commented Oct 10, 2017

Uh oh!

liketic commented Oct 10, 2017

Uh oh!

javanna commented Oct 10, 2017

Uh oh!

liketic commented Oct 10, 2017

Uh oh!

javanna commented Oct 10, 2017

Uh oh!

liketic commented Oct 11, 2017

Uh oh!

javanna commented Oct 11, 2017

Uh oh!

javanna commented Oct 11, 2017

Uh oh!

liketic commented Oct 14, 2017

Uh oh!

javanna commented Oct 23, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liketic commented Oct 27, 2017

liketic commented Sep 28, 2017 •

edited

Loading

javanna left a comment •

edited

Loading