From ac3e6d93fbb3690c08e864688a1acfa592a662c6 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 26 Jun 2017 12:43:54 +0100 Subject: [PATCH 1/7] 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 --- .../elasticsearch/search/builder/SearchSourceBuilder.java | 3 +++ .../search/builder/SearchSourceBuilderTests.java | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 99f74d1f8e0da..39c01cda8b0b6 100644 --- a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -346,6 +346,9 @@ public int from() { * The number of search hits to return. Defaults to 10. */ public SearchSourceBuilder size(int size) { + if (size < 0) { + throw new IllegalArgumentException("[size] parameter cannot be negative"); + } this.size = size; return this; } diff --git a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index f8bd7b80d0e9f..853898b069c03 100644 --- a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -365,6 +365,13 @@ public void testNegativeFromErrors() { assertEquals("[from] parameter cannot be negative", expected.getMessage()); } + public void testNegativeSizeErrors() { + IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-2)); + assertEquals("[size] parameter cannot be negative", expected.getMessage()); + expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-1)); + assertEquals("[size] parameter cannot be negative", 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(createParseContext(parser))); From b2386c63d40f2649c197d67834b366db81cebfb4 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 26 Jun 2017 13:40:59 +0100 Subject: [PATCH 2/7] fix error in reindex --- .../org/elasticsearch/search/builder/SearchSourceBuilder.java | 2 +- .../search/builder/SearchSourceBuilderTests.java | 2 +- .../index/reindex/AbstractBulkByQueryRestHandler.java | 4 +--- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 39c01cda8b0b6..8ab816781e8ec 100644 --- a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -347,7 +347,7 @@ public int from() { */ public SearchSourceBuilder size(int size) { if (size < 0) { - throw new IllegalArgumentException("[size] parameter cannot be negative"); + throw new IllegalArgumentException("[size] parameter cannot be negative, found " + size); } this.size = size; return this; diff --git a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 853898b069c03..2c8229b8a39c8 100644 --- a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -369,7 +369,7 @@ public void testNegativeSizeErrors() { IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-2)); assertEquals("[size] parameter cannot be negative", expected.getMessage()); expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-1)); - assertEquals("[size] parameter cannot be negative", expected.getMessage()); + assertEquals("[size] parameter cannot be negative, found -1", expected.getMessage()); } private void assertIndicesBoostParseErrorMessage(String restContent, String expectedErrorMessage) throws IOException { diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java index 64209653c6f4f..261e3843c8330 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java +++ b/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,7 @@ protected void parseInternalRequest(Request internal, RestRequest restRequest, SearchRequest searchRequest = internal.getSearchRequest(); int scrollSize = searchRequest.source().size(); - searchRequest.source().size(SIZE_ALL_MATCHES); + // searchRequest.source().size(SIZE_ALL_MATCHES); try (XContentParser parser = extractRequestSpecificFields(restRequest, bodyConsumers)) { RestSearchAction.parseSearchRequest(searchRequest, restRequest, parser); From eaf6265779eb7ef14a9722ff6cc9c6d7533cc649 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 3 Jul 2017 13:33:22 +0100 Subject: [PATCH 3/7] update re-index tests --- .../index/reindex/AbstractBulkByScrollRequest.java | 3 +++ .../elasticsearch/search/builder/SearchSourceBuilder.java | 2 +- docs/reference/migration/migrate_6_0.asciidoc | 3 +++ docs/reference/migration/migrate_6_0/reindex.asciidoc | 6 ++++++ .../rest-api-spec/test/delete_by_query/20_validation.yml | 2 +- .../rest-api-spec/test/update_by_query/20_validation.yml | 2 +- 6 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 docs/reference/migration/migrate_6_0/reindex.asciidoc diff --git a/core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java b/core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java index 5bfae5fde924a..641c76ada206e 100644 --- a/core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java +++ b/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(); } diff --git a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java index 8ab816781e8ec..0ab6e5ebcc498 100644 --- a/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java @@ -347,7 +347,7 @@ public int from() { */ public SearchSourceBuilder size(int size) { if (size < 0) { - throw new IllegalArgumentException("[size] parameter cannot be negative, found " + size); + throw new IllegalArgumentException("[size] parameter cannot be negative, found [" + size + "]"); } this.size = size; return this; diff --git a/docs/reference/migration/migrate_6_0.asciidoc b/docs/reference/migration/migrate_6_0.asciidoc index 22a698bd482d8..811439ab8ba62 100644 --- a/docs/reference/migration/migrate_6_0.asciidoc +++ b/docs/reference/migration/migrate_6_0.asciidoc @@ -31,6 +31,7 @@ way to reindex old indices is to use the `reindex` API. * <> * <> * <> +* <> * <> * <> * <> @@ -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[] diff --git a/docs/reference/migration/migrate_6_0/reindex.asciidoc b/docs/reference/migration/migrate_6_0/reindex.asciidoc new file mode 100644 index 0000000000000..d74a5102d91b7 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yml b/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yml index 7527db948422c..9daf1502a362f 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/delete_by_query/20_validation.yml +++ b/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 diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/20_validation.yml b/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/20_validation.yml index be83c0cb9f24f..8f8d492df3aa0 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/update_by_query/20_validation.yml +++ b/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 From 6821a09faf3e32a4ea912f0f3654c695dd96d312 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 3 Jul 2017 14:14:18 +0100 Subject: [PATCH 4/7] Addresses review comment --- .../index/reindex/AbstractBulkByQueryRestHandler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java index 261e3843c8330..e9973c9950053 100644 --- a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java +++ b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByQueryRestHandler.java @@ -50,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); From 4af626333f154a075438b7c14db28d0068dc7fc8 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 3 Jul 2017 14:19:35 +0100 Subject: [PATCH 5/7] Fixed tests --- .../index/reindex/AbstractBulkByScrollRequestTestCase.java | 4 +++- .../search/builder/SearchSourceBuilderTests.java | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequestTestCase.java b/core/src/test/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequestTestCase.java index debeab52d979d..8a255d376aff5 100644 --- a/core/src/test/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequestTestCase.java +++ b/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(); diff --git a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index 2c8229b8a39c8..dc690960d1555 100644 --- a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -367,9 +367,9 @@ public void testNegativeFromErrors() { public void testNegativeSizeErrors() { IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-2)); - assertEquals("[size] parameter cannot be negative", expected.getMessage()); + assertEquals("[size] parameter cannot be negative, found [-2]", expected.getMessage()); expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-1)); - assertEquals("[size] parameter cannot be negative, found -1", expected.getMessage()); + assertEquals("[size] parameter cannot be negative, found [-1]", expected.getMessage()); } private void assertIndicesBoostParseErrorMessage(String restContent, String expectedErrorMessage) throws IOException { From 75b3183158423f5ecf7b9c83598a962c3da4be6d Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 3 Jul 2017 14:57:16 +0100 Subject: [PATCH 6/7] Added random negative size test --- .../index/reindex/AbstractBulkByScrollRequest.java | 7 +++++-- .../search/builder/SearchSourceBuilderTests.java | 6 ++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java b/core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java index 641c76ada206e..9f10304622ba4 100644 --- a/core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java +++ b/core/src/main/java/org/elasticsearch/index/reindex/AbstractBulkByScrollRequest.java @@ -370,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. diff --git a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java index dc690960d1555..4b2599adda5c0 100644 --- a/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java @@ -366,8 +366,10 @@ public void testNegativeFromErrors() { } public void testNegativeSizeErrors() { - IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().size(-2)); - assertEquals("[size] parameter cannot be negative, found [-2]", expected.getMessage()); + 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()); } From e464e565baa76a75868afa89f992b4f4577da250 Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Mon, 3 Jul 2017 15:57:21 +0100 Subject: [PATCH 7/7] Fixes test --- .../java/org/elasticsearch/index/reindex/RoundTripTests.java | 4 +++- .../resources/rest-api-spec/test/reindex/20_validation.yml | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RoundTripTests.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RoundTripTests.java index a65463c5ed5c6..aadab54803c8c 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/RoundTripTests.java +++ b/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")); diff --git a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/20_validation.yml b/modules/reindex/src/test/resources/rest-api-spec/test/reindex/20_validation.yml index c9f441c9cd31d..b64eaac7dec22 100644 --- a/modules/reindex/src/test/resources/rest-api-spec/test/reindex/20_validation.yml +++ b/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: