Adds check for negative search request size (#25397)

* Adds check for negative search request size

This change adds a check to `SearchSourceBuilder` to throw and exception if the size set on it is set to a negative value.

Closes #22530

* fix error in reindex

* update re-index tests

* Addresses review comment

* Fixed tests

* Added random negative size test

* Fixes test

Author: colings86

core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java

@@ -171,6 +171,9 @@ public int getSize() {
      * documents.
      */
     public Self setSize(int size) {
+        if (size < 0) {
+            throw new IllegalArgumentException("[size] parameter cannot be negative, found [" + size + "]");
+        }
         this.size = size;
         return self();
     }
@@ -367,10 +370,13 @@ protected Self doForSlice(Self request, TaskId slicingTask) {
                 .setShouldStoreResult(false)
                 // Split requests per second between all slices
                 .setRequestsPerSecond(requestsPerSecond / slices)
-                // Size is split between workers. This means the size might round down!
-                .setSize(size == SIZE_ALL_MATCHES ? SIZE_ALL_MATCHES : size / slices)
                 // Sub requests don't have workers
                 .setSlices(1);
+        if (size != -1) {
+            // Size is split between workers. This means the size might round
+            // down!
+            request.setSize(size == SIZE_ALL_MATCHES ? SIZE_ALL_MATCHES : size / slices);
+        }
         // Set the parent task so this task is cancelled if we cancel the parent
         request.setParentTask(slicingTask);
         // TODO It'd be nice not to refresh on every slice. Instead we should refresh after the sub requests finish.

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

@@ -345,6 +345,9 @@ public int from() {
      * The number of search hits to return. Defaults to <tt>10</tt>.
      */
     public SearchSourceBuilder size(int size) {
+        if (size < 0) {
+            throw new IllegalArgumentException("[size] parameter cannot be negative, found [" + size + "]");
+        }
         this.size = size;
         return this;
     }

core/src/test/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequestTestCase.java

@@ -42,7 +42,9 @@ public void testForSlice() {
         original.setSlices(between(2, 1000));
         original.setRequestsPerSecond(
                 randomBoolean() ? Float.POSITIVE_INFINITY : randomValueOtherThanMany(r -> r < 0, ESTestCase::randomFloat));
-        original.setSize(randomBoolean() ? AbstractBulkByScrollRequest.SIZE_ALL_MATCHES : between(0, Integer.MAX_VALUE));
+        if (randomBoolean()) {
+            original.setSize(between(0, Integer.MAX_VALUE));
+        }
 
         TaskId slicingTask = new TaskId(randomAlphaOfLength(5), randomLong());
         SearchRequest sliceRequest = new SearchRequest();

core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java

@@ -359,6 +359,15 @@ public void testNegativeFromErrors() {
         assertEquals("[from] parameter cannot be negative", expected.getMessage());
     }
 
+    public void testNegativeSizeErrors() {
+        int randomSize = randomIntBetween(-100000, -2);
+        IllegalArgumentException expected = expectThrows(IllegalArgumentException.class,
+                () -> new SearchSourceBuilder().size(randomSize));
+        assertEquals("[size] parameter cannot be negative, found [" + randomSize + "]", expected.getMessage());
+        expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-1));
+        assertEquals("[size] parameter cannot be negative, found [-1]", expected.getMessage());
+    }
+
     private void assertIndicesBoostParseErrorMessage(String restContent, String expectedErrorMessage) throws IOException {
         try (XContentParser parser = createParser(JsonXContent.jsonXContent, restContent)) {
             ParsingException e = expectThrows(ParsingException.class, () -> SearchSourceBuilder.fromXContent(parser));

docs/reference/migration/migrate_6_0.asciidoc

@@ -31,6 +31,7 @@ way to reindex old indices is to use the `reindex` API.
 * <<breaking_60_aggregations_changes>>
 * <<breaking_60_mappings_changes>>
 * <<breaking_60_docs_changes>>
+* <<breaking_60_reindex_changes>>
 * <<breaking_60_cluster_changes>>
 * <<breaking_60_settings_changes>>
 * <<breaking_60_plugins_changes>>
@@ -55,6 +56,8 @@ include::migrate_6_0/mappings.asciidoc[]
 
 include::migrate_6_0/docs.asciidoc[]
 
+include::migrate_6_0/reindex.asciidoc[]
+
 include::migrate_6_0/cluster.asciidoc[]
 
 include::migrate_6_0/settings.asciidoc[]

docs/reference/migration/migrate_6_0/reindex.asciidoc

@@ -0,0 +1,6 @@
+[[breaking_60_reindex_changes]]
+=== Reindex changes
+
+==== `size` parameter
+
+The `size` parameter can no longer be explicitly set to `-1`. If all documents are required then the `size` parameter should not be set.

modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java

@@ -32,8 +32,6 @@
 import java.util.Map;
 import java.util.function.Consumer;
 
-import static org.elasticsearch.index.reindex.AbstractBulkByScrollRequest.SIZE_ALL_MATCHES;
-
 /**
  * Rest handler for reindex actions that accepts a search request like Update-By-Query or Delete-By-Query
  */
@@ -52,7 +50,6 @@ protected void parseInternalRequest(Request internal, RestRequest restRequest,
 
         SearchRequest searchRequest = internal.getSearchRequest();
         int scrollSize = searchRequest.source().size();
-        searchRequest.source().size(SIZE_ALL_MATCHES);
 
         try (XContentParser parser = extractRequestSpecificFields(restRequest, bodyConsumers)) {
             RestSearchAction.parseSearchRequest(searchRequest, restRequest, parser);

modules/reindex/src/test/java/org/elasticsearch/index/reindex/RoundTripTests.java

@@ -131,7 +131,9 @@ public void testDeleteByQueryRequest() throws IOException {
     private void randomRequest(AbstractBulkByScrollRequest<?> request) {
         request.getSearchRequest().indices("test");
         request.getSearchRequest().source().size(between(1, 1000));
-        request.setSize(random().nextBoolean() ? between(1, Integer.MAX_VALUE) : -1);
+        if (randomBoolean()) {
+            request.setSize(between(1, Integer.MAX_VALUE));
+        }
         request.setAbortOnVersionConflict(random().nextBoolean());
         request.setRefresh(rarely());
         request.setTimeout(TimeValue.parseTimeValue(randomTimeValue(), null, "test"));

modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yml

@@ -44,7 +44,7 @@
         id:      1
         body:    { "text": "test" }
   - do:
-      catch: /size should be greater than 0 if the request is limited to some number of documents or -1 if it isn't but it was \[-4\]/
+      catch: /\[size\] parameter cannot be negative, found \[-4\]/
       delete_by_query:
         index: test
         size: -4

modules/reindex/src/test/resources/rest-api-spec/test/reindex/20_validation.yml

@@ -104,7 +104,7 @@
         id:      1
         body:    { "text": "test" }
   - do:
-      catch: /size should be greater than 0 if the request is limited to some number of documents or -1 if it isn't but it was \[-4\]/
+      catch: /\[size\] parameter cannot be negative, found \[-4\]/
       reindex:
         body:
           source:

modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/20_validation.yml

@@ -21,7 +21,7 @@
         id:      1
         body:    { "text": "test" }
   - do:
-      catch: /size should be greater than 0 if the request is limited to some number of documents or -1 if it isn't but it was \[-4\]/
+      catch: /\[size\] parameter cannot be negative, found \[-4\]/
       update_by_query:
         index: test
         size: -4