Skip to content

Commit

Permalink
Check for negative "from" values in search request body (#54953)
Browse files Browse the repository at this point in the history
Today we already disallow negative values for the "from" parameter in the search
API when it is set as a request parameter and setting it on the
SearchSourceBuilder, but it is still parsed without complaint from a search
body, leading to differing exceptions later. This PR changes this behavior to be
the same regardless of setting the value directly, as url parameter or in the
search body. While we silently accepted "-1" as meaning "unset" and used the
default value of 0 so far, any negative from-value is now disallowed.

Closes #54897
  • Loading branch information
Christoph Büscher committed May 28, 2020
1 parent 6e64362 commit 3d4f9fe
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 12 deletions.
15 changes: 15 additions & 0 deletions docs/reference/migration/migrate_8_0/search.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,18 @@ removed in 8.0.
Discontinue use of the `use_field_mapping` request body parameter. Requests
containing this parameter will return an error.
====


.The search API's `from` request body and url parameter cannot be negative.
[%collapsible]
====
*Details* +
Search request used to accept `-1` as a `from` in the search body and the url,
treating it as the default value of 0. Other negative values got rejected with
an error already. We now also reject `-1` as an invalid value.
*Impact* +
Change any use of `-1` as `from` parameter in request body or url parameters by either
setting it to `0` or omitting it entirely. Requests containing negative values will
return an error.
====
2 changes: 1 addition & 1 deletion docs/reference/rest-api/common-parms.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ end::frequency[]

tag::from[]
`from`::
(Optional, integer) Starting document offset. Defaults to `0`.
(Optional, integer) Starting document offset. Needs to be non-negative and defaults to `0`.
end::from[]

tag::from-transforms[]
Expand Down
2 changes: 1 addition & 1 deletion docs/reference/search/search.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ If both parameters are specified, only the query parameter is used.
--

`size`::
(Optional, integer) The number of hits to return. Defaults to `10`.
(Optional, integer) The number of hits to return. Needs to be non-negative and defaults to `10`.
+
--
By default, you cannot page through more than 10,000 documents using the `from`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,33 @@ setup:
from: 10000

---
"Request with negative from value":
"Request with negative from value url parameter":

- skip:
version: " - 7.9.99"
reason: waiting for backport

- do:
catch: /\[from\] parameter cannot be negative/
catch: /\[from\] parameter cannot be negative but was \[-1\]/
search:
rest_total_hits_as_int: true
index: test_1
from: -2
from: -1

---
"Request with negative from value in body":

- skip:
version: " - 7.9.99"
reason: waiting for backport

- do:
catch: /\[from\] parameter cannot be negative but was \[-1\]/
search:
rest_total_hits_as_int: true
index: test_1
body:
from: -1

---
"Request window limits with scroll":
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,9 +181,8 @@ private static void parseSearchSource(final SearchSourceBuilder searchSourceBuil
searchSourceBuilder.query(queryBuilder);
}

int from = request.paramAsInt("from", -1);
if (from != -1) {
searchSourceBuilder.from(from);
if (request.hasParam("from")) {
searchSourceBuilder.from(request.paramAsInt("from", 0));
}
int size = request.paramAsInt("size", -1);
if (size != -1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,10 +336,11 @@ public QueryBuilder postFilter() {

/**
* From index to start the search from. Defaults to {@code 0}.
* Must be a positive value or 0.
*/
public SearchSourceBuilder from(int from) {
if (from < 0) {
throw new IllegalArgumentException("[from] parameter cannot be negative");
throw new IllegalArgumentException("[from] parameter cannot be negative but was [" + from + "]");
}
this.from = from;
return this;
Expand Down Expand Up @@ -1027,7 +1028,7 @@ public void parseXContent(XContentParser parser, boolean checkTrailingTokens) th
currentFieldName = parser.currentName();
} else if (token.isValue()) {
if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
from = parser.intValue();
from(parser.intValue());
} else if (SIZE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
size = parser.intValue();
} else if (TIMEOUT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.search.builder;

import com.fasterxml.jackson.core.JsonParseException;

import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
Expand Down Expand Up @@ -430,8 +431,9 @@ public void testParseIndicesBoost() throws IOException {
}

public void testNegativeFromErrors() {
IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().from(-2));
assertEquals("[from] parameter cannot be negative", expected.getMessage());
int from = randomIntBetween(-10, -1);
IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> new SearchSourceBuilder().from(from));
assertEquals("[from] parameter cannot be negative but was [" + from + "]", expected.getMessage());
}

public void testNegativeSizeErrors() {
Expand Down

0 comments on commit 3d4f9fe

Please sign in to comment.