Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,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 + "]");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can set the size to the default of 10, that's what SearchService#createContext defaults to later on anyway if it finds the current -1 value. I just find it easier to reason about if the value is set to the default in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to keep the default as-is here and let it be handled when creating the context as today so that we can distinguish between set and set from default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the set/unset distinction makes sense e.g. when using the builder on the client side, otherwise we would always render the size even if not set. What makes me not like this is the fact that we reject "-1" as illegal but still use it as a "legal" value meaning something internally. To me it feels a bit weird when you are just looking at this class.

this.size = size;
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe you could test -1 and another random negative int instead?

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());
}

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)));
Expand Down
3 changes: 3 additions & 0 deletions docs/reference/migration/migrate_6_0.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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>>
Expand All @@ -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[]
Expand Down
6 changes: 6 additions & 0 deletions docs/reference/migration/migrate_6_0/reindex.asciidoc
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down